JwtBearer argument validation

[Tratcher]

Same as #795

[kevinchalet]

Hem, what do you want to validate exactly?

[Tratcher]

I was testing something and it null refd on Options.ConfigurationManager. That said, ConfigurationManager seems to be optional, we just forgot to check for null in one place.

Are all the parameters really optional?

[kevinchalet]

I guess it was because of this line, which is not guarded against null values: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.JwtBearer/JwtBearerHandler.cs#L108

Are all the parameters really optional?

Yes (and one must be able to use the JWT bearer middleware without having an OIDC server)

Currently, Configuration and ConfigurationManager are both null if you use the JWT bearer middleware as a standalone (whereas the OIDC middleware would now throw in this case): the handler itself has a few null checks to make them optional.

In this case, it's up to the developer to provide the appropriate token validation parameters (e.g the issuer name).

[troydai]

It seems to me that Configuration and ConfigurationManager are both designed to be optional. I didn't find a place where the reference to them are not guarded. I may have missed, @Tratcher do you still remember what scenario caused the nullref? I tried miss both Authority and Audience properties, nullref didn't show.

Other then that, @Tratcher what else we should validate at the beginning of the server?

[Tratcher]

The null ref was in OIDC. I filed this one because I realized Jwt had no validation either. The biggest problem I see is that it still starts if you provide no arguments (e.g. your config file was accidentally empty), it just doesn't work. Try debugging through it to see how it fails in this scenario and if there's any way to validate against it. It's problematic that there are too many different ways to configure it.

[kevinchalet]

If you simply call app.UseJwtBearerAuthentication(), the error you'll ultimately get will likely be a SecurityTokenSignatureKeyNotFoundException:

Microsoft.IdentityModel.Tokens.SecurityTokenSignatureKeyNotFoundException: IDX10501: Signature validation failed. Unable to match 'kid': 'RAIWCEYISDAHJZUI7Z3EEZXDRUXIAYQCWE88LTZT'

It's a common trap so it would be great if a better error was returned, but I'm not sure there's an easy way to do that.

[kevinchalet]

The null ref was in OIDC.

On a related note, we should add more checks to ensure an authorization request or a logout request cannot be generated when the authorization/logout endpoint path resolved from the OIDC configuration or set from the RedirectToIdentityProvider event is null.

Currently, if the authorization endpoint is undefined, the challenge is applied to the current address (e.g ?response_type=id_token...), which can result in an endless loop. 3 OpenIddict users reported a similar behavior in their own apps.

You can easily reproduce it by cloning the OpenIddict MVC samples and updating the server sample to disable the authorization endpoint:

services.AddOpenIddict<ApplicationUser, IdentityRole<Guid>, ApplicationDbContext, Guid>()
    // .EnableAuthorizationEndpoint("/connect/authorize")
    // .EnableLogoutEndpoint("/connect/logout")
    .EnableTokenEndpoint("/connect/token")
    // .EnableUserinfoEndpoint("/connect/userinfo")

    // .AllowAuthorizationCodeFlow()
    // .AllowRefreshTokenFlow()
    .AllowPasswordFlow()

    .DisableHttpsRequirement();

[troydai]

OK. So sum it up:

Both authority (as MetadataAddress) and audience are expected. Exception will be thrown if either of them are null.

It could potential produce some test-ability issue. In some tests (like this one), set an Authority seems to shortcut the expected exception thrown from InvalidTokenValidator. Dealing with test-ability will be part of the change.

I ran into the missing redirect url issue brought up by @PinpointTownes previously, too. I'll open a separate issue to address it. (#903)

[kevinchalet]

Both authority (as MetadataAddress) and audience are expected. Exception will be thrown if either of them are null.

You mean for the OIDC middleware? Or does your remark also include the JWT middleware?

[troydai]

Only applies to Jwt middleware. since this issue is addressing Jwt Middleware

[kevinchalet]

Then it's important to note that making Authority mandatory would be a (major) breaking change, as it wouldn't allow one to use the JWT middleware without having an OIDC server: #844 (comment)

[troydai]

That means Authority is optional. Is there a condition in which missing of the Authority combining with other factor will cause major issues that we shouldn't allow server to start in the first place?

[kevinchalet]

The 'problem' with JWT is that there's no mandatory claim, so it's hard to make options like Audience mandatory, since access tokens don't have to expose an aud claim (IdentityModel will reject them by default, tho).

I guess you could update the JWT bearer middleware to throw an exception when Authority is null if no signing key is registered in the token validation parameters (which is the most common manifestation of an incorrect configuration), but it would prevent one from allowing non-signed tokens (which is extremely bad in practice, but allowed under certain circumstances: https://tools.ietf.org/html/rfc7519#section-6).

Not to mention that there are multiple ways to register a signing key: via TokenValidationParameters.IssuerSigningKey, TokenValidationParameters.IssuerSigningKeys, TokenValidationParameters.IssuerSigningKeyResolver and even IssuerSigningKeyResolver.IssuerSigningKeyValidator.

We might end up with a monstrous check:

if (Options.TokenValidationParameters.RequireSignedTokens &&
    string.IsNullOrEmpty(Options.MetadataAddress) &&
    Options.Configuration == null &&
    Options.ConfigurationManager == null &&
    Options.TokenValidationParameters.IssuerSigningKey == null &&
    Options.TokenValidationParameters.IssuerSigningKeyResolver == null &&
    Options.TokenValidationParameters.IssuerSigningKeys == null &&
    Options.TokenValidationParameters.IssuerSigningKeyValidator == null) {
    // Throw an exception, as this likely indicates an invalid configuration.
}

[troydai]

Thanks for the suggestion, @PinpointTownes . I'll look into this one.

[Tratcher]

So in the end there may not be any practical validation we can do. That's a shame because it's really easy to not add your config settings.

[Eilon]

We looked into this and due to the flexibility of the JWT provider, we can't find a good reliable way to do this type of validation.