AzureAD / microsoft-identity-web

Helps creating protected web apps and web APIs with Microsoft identity platform and Azure AD B2C
MIT License
683 stars 217 forks source link

[Bug] ValidIssuers is not applied #1360

Closed jc4gh closed 1 year ago

jc4gh commented 3 years ago

Which version of Microsoft Identity Web are you using? Microsoft Identity Web 1.15.1

Where is the issue?

Is this a new or an existing app? This is a new app or an experiment.

Repro Get a token from Tenant1 and call API.

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
                .AddMicrosoftIdentityWebApi(this.Configuration.GetSection("AzureAd"), subscribeToJwtBearerMiddlewareDiagnosticsEvents: true);

services.Configure(JwtBearerDefaults.AuthenticationScheme, (JwtBearerOptions options) =>
{
    var existingOnTokenValidatedHandler = options.Events.OnTokenValidated;
    options.Events.OnTokenValidated = async context =>
    {
        await existingOnTokenValidatedHandler(context);

        // Your code to add extra configuration that will be executed after the current event implementation.
        options.TokenValidationParameters.ValidIssuers = new[] { "https://sts.windows.net/[Tenant2 GUID]/" };
        options.TokenValidationParameters.ValidAudiences = new[] { "[audience]" };
    };
});

Expected behavior The request gets 401.

Actual behavior "Successfully validated the token." and passes validation.

Additional context / logs / screenshots Above code is from https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-protected-web-api-app-configuration#customizing-token-validation.

jennyf19 commented 3 years ago

@jc-msft try moving the condition for validating the token to before it's validated. that should resolve the issue.

services.Configure(JwtBearerDefaults.AuthenticationScheme, (JwtBearerOptions options) =>
{
     // Your code to add extra configuration that will be executed after the current event implementation.
     options.TokenValidationParameters.ValidIssuers = new[] { "https://sts.windows.net/[Tenant2 GUID]/" };
     options.TokenValidationParameters.ValidAudiences = new[] { "[audience]" };

     var existingOnTokenValidatedHandler = options.Events.OnTokenValidated;

     options.Events.OnTokenValidated = async context =>
     {
          await existingOnTokenValidatedHandler(context);
     };
});
jennyf19 commented 3 years ago

i believe this is resolved. closing, but feel free to reopen

jc4gh commented 3 years ago

It still doesn't seem to validate. Token: image

Code: image

image

jc4gh commented 3 years ago

@jennyf19 I can't seem to re-open.

jpda commented 3 years ago

Hey @jennyf19 I'm having a similar issue - it seems like, even when setting ValidIssuers, the https://login.microsoftonline.com/{tenant}/v2.0 issuer is still being added to the ValidIssuers list after configuration, presumably from the openid connect metadata.

For example - when the merged options are created, and when they are assigned to openid options, only my two issuers (from the ValidIssuers config) are in the list.

image

But at runtime, when the aad issuer validator runs, the list of valid issuers also includes the metadata value:

image

Presumably, if the user provided a list of valid issuers, they would like to ignore the metadata issuer and checking the issuer against the ValidIssuers list only and skipping (or even removing) the metadata-fetched templated issuer would have a desired result, but I'm not entirely certain what side-effects that would have.

jennyf19 commented 3 years ago

@jpda can you send me a repro? thx.

jpda commented 3 years ago

here you go @jennyf19 https://github.com/jpda/restricted-issuer

jennyf19 commented 3 years ago

@jpda thanks so much. here you need to configure the OpenIdConnectOptions not the MicrosoftIdentityOptions and just set either ValidIssuer or ValidIssuers but not both.

jpda commented 3 years ago

Ah ok - I had ValidIssuer in there to check for something else (that is a bad issuer value in the ValidIssuer anyway).

OK - so I have OpenIdConnectOptions as the only bit of configuration here

image

However the valid issuer list seems to still include the one from metadata.

image

Even if the issuer isn't in the valid list, this block would fetch metadata and get that templated issuer value from metadata, which would bypass the ValidIssuers list

I have a rather inelegant experiment here: https://github.com/jpda/microsoft-identity-web/blob/1809d6a3cd1ace35025c7dcde1c73264b7405dc8/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs#L78

and maybe a little better of the same idea here: https://github.com/jpda/microsoft-identity-web/blob/7140e3ce461c31ee5b61d22db9982c574edad6e3/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs#L81

thecodetinker commented 2 years ago

I was very glad to stumble across this issue, because I've been trying for hours to understand why my code has stopped working after migrating to Microsoft.Identity.Web! Can you please recommend a workaround until this is fixed (hopefully soon - seems like a pretty major security hole)? Something like the following maybe?


services
        .AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
        .AddMicrosoftIdentityWebApp(options =>
        {
          options.Events = new OpenIdConnectEvents
          {
            OnTokenValidated = async context =>
            {
              await existingOnTokenValidatedHandler(context);

              if (validIssuers.Length > 0 && !validIssuers.Any(iss => iss == context.SecurityToken.Issuer))
              {
                context.HandleResponse();
                context.Response.Redirect($"{baseUrl}/Account/AccessDenied");
              }
            }
          };
        });
`
jmprieur commented 2 years ago

@jpda @thecodetinker I investigated, and it turns out that ASP.NET Core always adds an issuer to the TokenValidationParameters.Issuers. The Issuer which is fetched from the OpenIdConnectConfiguration (From the issuer property of the https://login.microsoftonline.com/{tenant}/v2.0/.well-known/openid-configuration metadata document)

Then the other ValidIssuers are appended to this one.

This is happening here:

https://github.com/dotnet/aspnetcore/blob/a450cb69b5e4549f5515cdb057a68771f56cefd7/src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs#L101

In order to avoid this behavior, I recommend that you also override the IssuerValidator delegate to match the issuer with only your valid issuers.

I'm going to start a conversation with the ASP.NET Core team to understand if there is a better option. cc: @tratcher

thecodetinker commented 2 years ago

Hi again and thanks for the update. I'm a bit unsure about overriding the IssuerValidator delegate - from what I can see the default does a lot of checking that would need replicating. Did you have any luck with the ASP.NET Core team, or the possible solutions proposed by @jpda ?

jmprieur commented 2 years ago

@tratcher do we want to do something about the fact that ASP.NET Core systematically adds the ClientId as a valid issuer? (note that this would be a breaking change)