Aspnetcore: OIDC handler running on '/' crashes even with SkipUnrecognizedRequests set to true

[mderriey]

Describe the bug

The OIDC handler throws an exception related to correlation validation when it's running on an endpoint that is not dedicated to receiving OIDC messages. While I understand it's not the recommendation (the XML doc for SkipUnrecognizedRequests makes it really clear), UI suppose there might be instances where one doesn't have control over the configured redirect URL.

To Reproduce

Steps to reproduce the behavior:

1. Create a brand new VS 2019 ASP.NET Core MVC web application
2. Add the OIDC handler with CallbackPath set to / and SkipUnrecognizedRequests set to true
3. Start the app
4. The DeveloperExceptionPageMiddleware gets triggered and shows the exception message "Correlation failed."

Expected behavior

The OIDC handler should ignore requests that don't contain an OIDC message when SkipUnrecognizedRequests is true.

Additional context

I've done some debugging and think I found where this comes from:

1. L508 - the AuthenticationProperties come back null from ReadPropertiesAndClearState since there's no state in the OIDC message — there's no OIDC message at all
2. L510 - we then run RunMessageReceivedEventAsync
3. L1039 - we new up an instance of MessageReceivedContext passing in the null AuthenticationProperties
4. MessageReceivedContext inherits RemoteAuthenticationContext<T> which creates a new instance of AuthenticationProperties if the one provided is null, see https://github.com/aspnet/AspNetCore/blob/d3400f7cb23d83ae93b536a5fc9f46fc2274ce68/src/Security/Authentication/Core/src/Events/RemoteAuthenticationContext.cs#L28
5. we then assign the Properties from the MessageReceivedContext to our previously null properties variable, see https://github.com/aspnet/AspNetCore/blob/d3400f7cb23d83ae93b536a5fc9f46fc2274ce68/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L516
6. then we skip the two opportunities to bail out from the request here and there
7. we then blow up when trying to validate the correlation id at https://github.com/aspnet/AspNetCore/blob/d3400f7cb23d83ae93b536a5fc9f46fc2274ce68/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L548

Workaround

The current workaround is to handle the MessageReceived event and do some manual checking on either the OIDC message or the properties and bail out with HandleRequestResult.SkipHandler().

Fix

I'd be happy to open a PR for that one if you decide this is worth fixing.

[Tratcher]

And why is CallbackPath set to /? That's even worse than using SkipUnrecognizedRequests.

[mderriey]

In that case, the AAD app registration I was trying to use had been set up with that redirect URL. I totally understand it's not the recommendation and the preference is to have an endpoint dedicated to receiving callbacks from the IdP.

I thought SkipUnrecognizedRequests was created for those cases where the CallbackPath is not dedicated to OIDC and can be invoked for other requests.

[Tratcher]

In theory yes, though it was more for the case where there were multiple OIDC instances on the same endpoint. Having it inspect every request to your home page is not going to end well. Get the app registration updated.

You're welcome to try a PR.

[lovelangy]

And why is CallbackPath set to /? That's even worse than using SkipUnrecognizedRequests.

hi @Tratcher, Can you please explain the reason why setting callbackpath as / is a bad idea?

I run into the same senario when using WS-Federation. i have a legacy ASP.NET website, which using WS-Federation Authentication Module and the reply has bee set as '/'

After we migrated it to ASP.NET CORE, it doesn't work. For some of reason, i cannot get the app registration updated to '/signin-wsfed' (i don't manage the server)

So I have to create my own WsFederationHandler.cs and override the below method and make it work.

public virtual Task<bool> ShouldHandleRequestAsync()
            => Task.FromResult(Options.CallbackPath == Request.Path);

https://github.com/aspnet/AspNetCore/blob/master/src/Security/Authentication/Core/src/RemoteAuthenticationHandler.cs#L41

[mderriey]

Hey I think I can answer that question.

It's generally a bad idea because it means there'll be extra processing on all requests to / as the WS-Fed authentication handler will try to interpret these requests as coming back from your IdP. This means trying to find a relevant message, decode it, and all the rest (see the HandleRemoteAuthenticateAsync method).

Most of your requests to / will have nothing to do with authentication, which is why it's highly recommended to use a separate endpoint as a redirect URL for the IdP, so it doesn't introduce noise in unrelated requests.

Does that make sense?

[Tratcher]

Well said @mderriey

[lovelangy]

@mderriey thanks for your explaination. but the turth is the reqeust coming back from Idp is a HTTP Post, a generic request to / is a HTTP Get; and HandleRemoteAuthenticateAsync only interpret POST, for GET it will throw a exception as below . End user doesn't even be able to access home page.

System.Exception: No message.
Exception: An error was encountered while handling the remote login.
Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler<TOptions>.HandleRequestAsync()
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

what i have done is override ShouldHandleRequestAsync method,so that authentication handler ignores GET request to /

 public virtual Task<bool> ShouldHandleRequestAsync()
            => Task.FromResult(Options.CallbackPath == Request.Path);

to

 public overrideTask<bool> ShouldHandleRequestAsync()
            => Task.FromResult(Options.CallbackPath == Request.Path  && HttpMethods.IsPost(Request.Method));

@Tratcher
My understanding is that ASP.NET CORE as a framework should only handle the request it should do(HTTP POST + callbackpath), and leave other request(HTTP GET+ callbackpath) to appliation layer.

There are a lot of legacy ASP.NET website with '/' as callbackpath, for backward compatibility consideration, leaving the decision(whether to set callbackpath as '/' or not ) to application developer may be better idea.

I can create a PR if you agree.

[mderriey]

Hi @lovelangy

All remote authentication providers assume by default that requests they handle originate from endpoints that are dedicated to a response from the IdP.

In your case, the exception you're seeing comes from here.

IMO, a better alternative than to subclass the handler would be to set the SkipUnrecognizedRequests property on the WsFederationOptions to true.

That'll instruct the authentication handler to ignore requests from which a message cannot be extracted, hence the request will flow to the application layer.

[lovelangy]

@mderriey Thanks for quick reponse. Yes, you are correct. Instead of subclass the hanlder, I should use SkipUnrecognizedRequests. I did not notice this option before.