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

Initializing Azure SignalR using UseAzureSignalR doesn't facilitate examining Authorization metadata soon enough like UseEndpoints does. #1602

Closed
mhintzke opened this issue May 1, 2022 · 3 comments · Fixed by #1605

Comments

@mhintzke
Copy link

mhintzke commented May 1, 2022

Describe the bug

Using UseAzureSignalR does not inspect the Hub objects for AuthorizeAttributes prior to making the /negotiate call like ASP .NET Core SignalR does. This results in Negotiation authorization to behave incorrectly. Authorization for Azure SignalR only takes place between the negotiate calls and the redirection to Azure SignalR, but if Authorization to /negotiate fails, then the Authorization check against the Hubs can never happen.

How does it work in ASP .NET SignalR?

In ASP .NET Core SignalR, after a hub is started and a negotiation takes place, the DefaultHubDispatcher calls a method called IsHubMethodAuthorized here:

https://github.com/dotnet/aspnetcore/blob/0ee742c53f2669fd7233df6da89db5e8ab944585/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs#L568

This uses the HubMethodDescriptor.Policies to determine if the user is Authorized to perform negotiation. Those same HubMethodDescriptor.Policies are accumulated using the following code:

https://github.com/dotnet/aspnetcore/blob/0ee742c53f2669fd7233df6da89db5e8ab944585/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs#L658

This looks at the Hubs, and grabs the AuthorizeAttributes off each to build up a list of IAuthorizeData (policies).

This allows ASP .NET Core to properly handle Authorization of Hubs exactly like Authorization of standard controller endpoints. This logic should probably be used in Azure SignalR as well so that policies can be enforced (or not enforced) in the same way.

I will add, I think the primary place that this metadata gets added to the /negotiate endpoints is right here:

https://github.com/dotnet/aspnetcore/blob/de3019b7dab5b5995942db1fbec6aa466c4af34c/src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs#L118

To Reproduce

See this forked branch -> https://github.com/mhintzke/azure-signalr/tree/matt/reproduce-1602

Just add a connection string and then toggle ShouldUseAzureSignalR app setting between true and false and load the app. You will see that when using Azure SignalR we get a 401 on the /negotiate (because the FallbackPolicy is used) but on ASP .NET SignalR we get a 200.


Simply setup a FallbackPolicy that will always fail authorization and a "foobar" policy (it can do anything, but you can set a breakpoint on this and see that it is never be invoked) using native ASP .NET AddAuthorization method. Then, add [Authorize(Policy = "foobar")] to a Hub and try to negotiate with it. You will see that rather than using "foobar" to Authorize, it uses the FallbackPolicy and will therefore fail Authorization. Setting FallbackPolicy back to null then allows the /negotiate endpoint to pass Authorization upstream from Azure SignalR endpoint and then the Hub authorizes correctly.

Further technical details

  • Azure SignalR v1.17.0 (also reproduced on 1.15.0)
  • .NET 6 (but will reproduce in .NET 5 since .NET 6 doesn't seem to be officially supported)
mhintzke added a commit to mhintzke/azure-signalr that referenced this issue May 2, 2022
@mhintzke
Copy link
Author

mhintzke commented May 3, 2022

After further review (when reproducing in sample), I have realized that the expected behavior occurs when using IApplicationBuilder.UseEndpoints instead of IApplicationBuilder.UseAzureSignalR. The former properly sets up the IAuthorizeData as endpoint metadata for the Authorization system to look at. The latter does not. So rather than being a bug, maybe this just needs to be deprecated?

Is there a valid use case for UseAzureSignalR in your mind?

@mhintzke mhintzke changed the title Initializing a hub connection doesn't facilitate an Authorization check soon enough like ASP .NET Core SignalR does. Initializing a Azure SignalR using UseAzureSignalR doesn't facilitate an Authorization check soon enough like UseEndpoints does. May 3, 2022
@vicancy
Copy link
Member

vicancy commented May 5, 2022

Sounds a bug to look into.

@mhintzke mhintzke changed the title Initializing a Azure SignalR using UseAzureSignalR doesn't facilitate an Authorization check soon enough like UseEndpoints does. Initializing Azure SignalR using UseAzureSignalR doesn't facilitate an Authorization check soon enough like UseEndpoints does. May 5, 2022
@mhintzke mhintzke changed the title Initializing Azure SignalR using UseAzureSignalR doesn't facilitate an Authorization check soon enough like UseEndpoints does. Initializing Azure SignalR using UseAzureSignalR doesn't facilitate examining Authorization metadata soon enough like UseEndpoints does. May 5, 2022
@xingsy97
Copy link
Contributor

xingsy97 commented May 10, 2022

Thanks for your detailed feedback. This issue will be solved by #1605 , which obsoletes UseAzureSignalR when .NET Core version >= 3.1

We find that UseSignalR behaves same as UseAzureSignalR and your reproduce is still work using UseSignalR. UseSignalR follows global auth and doesn't support customized auth policy. So we choose to obsolete UseAzureSignalR rather than updating it.

Endpoint routing is adopted by SignalR since ASP.NET Core 3.0 and supports customized auth policy. After that, UseSignalR was marked as obsolete. In .NET Core 5.0, UseSignalR was deleted according to this.

UseAzureSignalR will be marked as obsolete when .Net Core version > 3.1 and we recommend UseEndpoints as alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants