Skip to content
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

ClaimsIdentity.Label not preserved in CookieAuth #22349

Closed
Vaccano opened this issue May 29, 2020 · 7 comments
Closed

ClaimsIdentity.Label not preserved in CookieAuth #22349

Vaccano opened this issue May 29, 2020 · 7 comments
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved

Comments

@Vaccano
Copy link

Vaccano commented May 29, 2020

Describe the bug

When setting up OpenID Connect in a new ASP.Net Core 3.1 application you can call

services.AddAuthentication().AddOpenIdConnect(openIdConnectOptions=> 
{
  // Configuration Code Here
});

In there, you can wire up to events like this:

openIdConnectOptions.Events = new OpenIdConnectEvents
{
    OnTokenValidated = context =>
    {
        if (context.Principal.Identity is ClaimsIdentity claimsIdentity)
        {
            claimsIdentity.Label = "MyCustomLabelHere";
        }

        return Task.CompletedTask;
    }
}

When setting the label on the ClaimsIdentity object in the OnTokenValidated event, the label does not stay changed. (It is set to null on the User.Identities object in the Controller.)

To Reproduce

  1. Create a new ASP.Net Core 3.1 MVC application and set it up to use OpenID Connect normally.
  2. Add a OnTokenValidated event and in it change the label of the ClaimsIdentity object (As shown above).
  3. Put a breakpoint in the HomeController.Index method run the application.
  4. Load the Home page.
  5. When the break point is hit, add a debugger watch of User.Identities.First().Label.

Expected: The watch value of Label will show the value set in the OnTokenValidated event.
Actual: The watch value of Label is set to null.

NOTE: There are other similar places where this works correctly (ie it stays changed). Namely:

  • AddJwtBearer 's OnTokenValidated event
  • AddCookie's OnValidatePrincipal event

But for some reason it does not stay changed for the AddOpenIdConnect's OnTokenValidated event.

Further technical details

  • ASP.NET Core version: 3.1
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version: Visual Studio 2019 (v16.5.4)
  • Include the output of dotnet --info
Click to expand output of `dotnet --info` .NET Core SDK (reflecting any global.json): Version: 3.1.201 Commit: b1768b4ae7

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18362
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.1.201\

Host (useful for support):
Version: 3.1.3
Commit: 4a9f85e9f8

.NET Core SDKs installed:
2.1.505 [C:\Program Files\dotnet\sdk]
2.1.509 [C:\Program Files\dotnet\sdk]
2.1.602 [C:\Program Files\dotnet\sdk]
2.1.604 [C:\Program Files\dotnet\sdk]
2.1.801 [C:\Program Files\dotnet\sdk]
2.2.202 [C:\Program Files\dotnet\sdk]
2.2.204 [C:\Program Files\dotnet\sdk]
2.2.401 [C:\Program Files\dotnet\sdk]
3.0.100-preview8-013656 [C:\Program Files\dotnet\sdk]
3.0.100-preview9-014004 [C:\Program Files\dotnet\sdk]
3.1.201 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.0.0-preview8.19405.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.0-preview8-28405-07 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.0.0-preview8-28405-07 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

@javiercn javiercn added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label May 29, 2020
@Tratcher Tratcher self-assigned this May 29, 2020
@Tratcher
Copy link
Member

What's the Label property used for? This is the first time I've seen anyone ask for it.

The reason this isn't preserved is because OIDC's ClaimsIdentity's are serialized into the auth cookie since OnTokenValidated and HomeController.Index happen on different requests. That serializer doesn't preserve the Label property. I don't think this was intentional, just an oversite.

protected virtual void WriteIdentity(BinaryWriter writer, ClaimsIdentity identity)
{
if (writer == null)
{
throw new ArgumentNullException(nameof(writer));
}
if (identity == null)
{
throw new ArgumentNullException(nameof(identity));
}
var authenticationType = identity.AuthenticationType ?? string.Empty;
writer.Write(authenticationType);
WriteWithDefault(writer, identity.NameClaimType, ClaimsIdentity.DefaultNameClaimType);
WriteWithDefault(writer, identity.RoleClaimType, ClaimsIdentity.DefaultRoleClaimType);
// Write the number of claims contained in the identity.
writer.Write(identity.Claims.Count());
foreach (var claim in identity.Claims)
{
WriteClaim(writer, claim);
}
var bootstrap = identity.BootstrapContext as string;
if (!string.IsNullOrEmpty(bootstrap))
{
writer.Write(true);
writer.Write(bootstrap);
}
else
{
writer.Write(false);
}
if (identity.Actor != null)
{
writer.Write(true);
WriteIdentity(writer, identity.Actor);
}
else
{
writer.Write(false);
}
}

The other examples you cited aren't affected because you're only ever checking the result on the same request.

@Tratcher Tratcher removed their assignment May 29, 2020
@Vaccano
Copy link
Author

Vaccano commented Jun 2, 2020

@Tratcher -

>>What's the Label property used for?

I have different types of JWTs. Each one has different values in the payload. I use the Label to determine which one I am dealing with.

In the cookie, I don't think I will ever have more than one type (a user based JWT). But my code I feed the JWT into wants to know the type so I can look up values in the JWT correctly. (My service code can get a call from a user JWT or an application JWT. The user JWT is generated by the IDP, the applicaiton JWT is generated by the API Gateway.)

>> I don't think this was intentional, just an oversite.

I can work around this by setting the Label in the OnValidatePrincipal event, but it is not the ideal location for it (it has to be re-set for each request).

Is this something that could be added into a future release?

@Tratcher
Copy link
Member

Tratcher commented Jun 2, 2020

Is this something that could be added into a future release?

It's possible. We're very careful about changing the cookie serialization format as that causes interop issues with older versions. Adding an extra field at the end should be relatively safe though, we might not even bump the version.

@Tratcher Tratcher added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jun 2, 2020
@Tratcher Tratcher changed the title Changes to ClaimsIdentity.Label in OpenIdConnectOptions.Events.OnTokenValidated are Lost ClaimsIdentity.Label not preserved in Auth Cookie Jun 2, 2020
@Tratcher Tratcher changed the title ClaimsIdentity.Label not preserved in Auth Cookie ClaimsIdentity.Label not preserved in CookieAuth Jun 2, 2020
@blowdart
Copy link
Contributor

blowdart commented Jun 4, 2020

Would using the issuer in this case work for you as a work around?

@kevinchalet
Copy link
Contributor

I don't think this was intentional, just an oversite.

Not exactly an oversight. Nobody pushed for supporting it: aspnet/Security#465 (comment)

@Tratcher
Copy link
Member

Tratcher commented Jun 5, 2020

Lol. It looks like we completely missed your question when you asked about it @kevinchalet.

@blowdart
Copy link
Contributor

Including the field would be a large breaking change, and would break compat with all previous versions. Given that I haven't seen an explanation of why label is special, over, say, a custom claim, it's simply too risky to do.

Closing as by design.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 11, 2020
@danroth27 danroth27 added the ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. label Jul 20, 2020
@ghost ghost added the Status: Resolved label Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants