Azure / azure-functions-host

The host/runtime that powers Azure Functions
https://functions.azure.com
MIT License
1.94k stars 441 forks source link

`ClaimsPrincipal.IsInRole` doesn't work with AAD application roles #3898

Open johndowns opened 5 years ago

johndowns commented 5 years ago

When using the EasyAuth ClaimsPrincipal binding with an Azure AD token with an application role, the ClaimsPrincipal.IsInRole() method always returns false even when the token has the role included.

Investigative information

Repro steps

  1. Create a new function app. Enable EasyAuth through the portal.
  2. Update the app registration in Azure AD to add a new app role to the app's manifest, such as:
    {
    "allowedMemberTypes": [
        "Application"
    ],
    "description": "Administrators can manage the Surveys in their tenant",
    "displayName": "SurveyAdmin",
    "id": "c20e145e-5459-4a6c-a074-b942bbd4cfe1",
    "isEnabled": true,
    "value": "SurveyAdmin"
    }
  3. Create a client app in the same AAD tenant, create a key, assign the role from step 2 to the client app, and grant the permission.
  4. Create a function in the portal with the following code:
#r "Newtonsoft.Json"

using System.Security.Claims;
using System.Linq;
using System.Net;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Primitives;
using Newtonsoft.Json;

public static async Task<IActionResult> Run(HttpRequest req, ClaimsPrincipal principal, ILogger log)
{
    log.LogInformation($"Found {principal.Identities.Count()} identities.");
    foreach (var identity in principal.Identities)
    {
        log.LogInformation($"Identity {identity.Name}:");
        log.LogInformation($"Auth type is {identity.AuthenticationType}");
        foreach (var claim in identity.Claims)
        {
            log.LogInformation($"Claim '{claim.Type}' = '{claim.Value}'");
        }
    }

    var isInRole = principal.IsInRole("SurveyAdmin");
    log.LogInformation($"Principal is {(isInRole ? "" : "NOT")} in the SurveyAdmin role.");

    return new OkResult();
}
  1. Obtain an Azure AD token. Observe that the token includes the roles claim, which is a string array with a single item (SurveyAdmin).
  2. Send a request to the function, attaching the token. Observe that the role claim is included in the enumerated list of claims logged out, but the log also includes Principal is NOT in the SurveryAdmin role. since the IsInRole method has returned false.

Expected behavior

The roles claim contains the roles associated with that principal. It should be honoured when evaluating the IsInRole method. (Also, on the main EasyAuth issue, @ConnorMcMahon indicated that this specific scenario should work.)

Actual behavior

The IsInRole method returns false.

Known workarounds

By manually inspecting the claims associated with the identity, we can perform our own version of the IsInRole logic.

Related information

ConnorMcMahon commented 5 years ago

Thanks for bringing this to my attention. I will investigate this scenario.

ConnorMcMahon commented 5 years ago

So it turns out that EasyAuth is using an older version of System.IdentityModels.Tokens.Jwt that doesn't have the "roles" claim mapped to "http://schemas.microsoft.com/ws/2008/06/identity/claims/role", which is why ClaimsPrincipal.IsInRole doesn't detect it.

We have a potential release of EasyAuth that has updated System.IdentityModels.Tokens.Jwt, but this would likely be a breaking change, as some people may have already written code against the "roles" claim. We are working on a way to make this work without causing a breaking change.

myusrn commented 5 years ago

Good to see there is an issue open against this matter as I got hung up for a good chunk of time assuming principal.IsInRole() was working before I broke down and added a claims dump to log output given issue #3810 where i'm unable to use attach debugger..

The work around i'm using is to use both the out of box role check and a current function app ClaimsPrincipal claim set compatible one for my role checks. While this does lead to unnecessary code it should not be susceptible to breaking if the "roles" array in issued tokens eventually gets mapped to a set of ClaimTypes.Role claims in ClaimsPrincipal.

// see https://github.com/Azure/azure-functions-host/issues/3898
public static bool IsInRoleFuncApp(this ClaimsPrincipal principal, string roleName)
{
    const string FunctionAppPrincipalRoleClaimType = "roles"; // vs ClaimTypes.Role == "http://schemas.microsoft.com/ws/2008/06/identity/claims/role"
    foreach(Claim claim in principal.FindAll(FunctionAppPrincipalRoleClaimType)) { if (claim.Value == roleName) return true; }
    return false;
}

[FunctionName("Interpret")]
        public static async Task<IActionResult> Run( . . . )
{
    . . .
    log.LogInformation($"the current user is {principal.Identity.Name} with isAuthenticated = {principal.Identity.IsAuthenticated}");
    //foreach (var claim in principal.Claims) { log.LogInformation($"current user has claim type = {claim.Type} with value = {claim.Value}"); }
    if (principal.IsInRole("Sales")) log.LogInformation($"the current user {principal.Identity.Name} IsInRole(\"Sales\") check returned true");
    if (principal.IsInRole("Finance")) log.LogInformation($"the current user {principal.Identity.Name} IsInRole(\"Finance\") check returned true");
    if (principal.IsInRoleFuncApp("Sales")) log.LogInformation($"the current user {principal.Identity.Name} IsInRoleFuncApp(\"Sales\") check returned true");
    if (principal.IsInRoleFuncApp("Finance")) log.LogInformation($"the current user {principal.Identity.Name} IsInRoleFuncApp(\"Finance\") check returned true");
ChristianWeyer commented 5 years ago

Do you have an update on this @ConnorMcMahon ? Thanks!

ConnorMcMahon commented 5 years ago

Thanks for pinging me on this.

I don't think we can make a fix in the core Authentication/Authorization feature, as customers have developed code around the claims that it has always had present.

However, I can add some special logic in the Functions runtime to manually edit the ClaimsPrincipal to add a Claim for "roles" that has the same value as the claim for "http://schemas.microsoft.com/ws/2008/06/identity/claims/role".

I will avoid actually removing the claim for "http://schemas.microsoft.com/ws/2008/06/identity/claims/role", so I don't break workarounds like the one shown by @myusrn above.

myusrn commented 5 years ago

With the code i used it wouldn't break if the role claims started being unwound into expected ClaimTypes.Role == "http://schemas.microsoft.com/ws/2008/06/identity/claims/role" entries and Principal.IsInRole("RoleName") started working. I would just have easily searchNremove dead code to strip out once this was the case.

jimpudar commented 4 years ago

@ConnorMcMahon, any update on the special logic in the Functions runtime? I still don't seem to be able to use IsInRole properly with tokens from AAD.

DL444 commented 3 years ago

I want to point out that this issue is not limited to Azure Functions. Any token returned by Azure AD authentication service that has an App Role specified will have the same issue.

SvenSowa commented 2 years ago

Can you update us on the matter? It seems still an issue even in a simple asp.net app running on an App Service.

stewartbright commented 2 years ago

I just ran into this while re-deploying a Function app after a period of time. In the previous deployment, my application roles were presented in JWT claims as "http://schemas.microsoft.com/ws/2008/06/identity/claims/role", and IPrincipal.IsInRole worked as expected. After trying to redeploy with literally no code changes, it wasn't working any more because application roles now present as schemaless "roles" (plural), and I had to change usage of IPrincipal.IsInRole to ClaimsPrincipal.HasClaim ("roles", ...) - an unnecessary reference to a concrete class.

I understand not wanting to break people's recent changes, but along the way somebody already broke it and the way it's working now is just wrong, IMO.

At the very least, "roles" should be a member of ClaimTypes and the IsInRole method should treat "Role" and "Roles" the same...