RequireAuthorization doesn't work as expected

[WolfspiritM]

One of the big features of the new endpoint routing in asp.net core 3 is the possibility to specify Authorization for each endpoint.

With the current implementation for MapHealthChecksUI() the following doesn't work and only blocks one of the routes (the webhook one).

                endpoints.MapHealthChecksUI().RequireAuthorization("policy");

It would be great if this can be made possible cause we don't want the health-ui endpoint to be open to the public.

I think this would require the api and webhook endpoints to be located under the "main" endpoint path e.g.: Main path -> "/healthcheck-ui", Api path -> "/healthcheck-ui/api", Webhook path -> "/healthcheck-ui/webhook"

... but it would make configuring way easier cause metadata (like authorization) is added to the whole endpoint and not just one of the routes.

[unaizorrilla]

Umm, god catch!

Let me check if we can do something on this!

[CarlosLanderas]

@WolfspiritM you can configure all UI paths using the setup of MapHealthChecksUI Can you try configuring them under the same segment and tell us if that works for you?

config.MapHealthChecksUI(setup =>
                    {                       
                        setup.UIPath = "/healthcheck-ui"
                        setup.WebhookPath = "/healthcheck-ui/webhook"
                        setup.ResourcesPath = "/healthcheck-ui/resources"
                        setup.ApiPath =  "/healthcheck-ui/api"
                    });

[WolfspiritM]

@CarlosLanderas Thank you, but this isn't the problem. Yes you can configure the path. But this issue is not about paths but about security and authorization. The new Endpoint system has a feature to add ".RequireAuthorization" to the endpoint to force it using Authorization or ".RequireHost" to make it require a specific host. This doesn't work in this case as only one of the routes gets blocked. This is not how the endpoint routing is supposed to work.

Even setting them under the same segment won't work for setting authorization as far as I can think. The problem is that MapHealthChecksUI returns only one Endpoint here: https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/bc0f481404412015aa11aeb30709eebba5dcd9dc/src/HealthChecks.UI/EndpointRouteBuilderExtensions.cs#L37

The others aren't returned: https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/bc0f481404412015aa11aeb30709eebba5dcd9dc/src/HealthChecks.UI/EndpointRouteBuilderExtensions.cs#L36

So when doing a MapHealthChecksUI(..).RequireAuthorization(..) it's only applied to the one returned but not the others.

To fix this I think the right way is to return a custom Convention Builder like this:

    class HealthCheckUIConventionBuilder : IEndpointConventionBuilder
    {
        private readonly IEndpointConventionBuilder apiEndpoint;
        private readonly IEndpointConventionBuilder webhookEndpoint;
        private readonly IEndpointConventionBuilder resourceEndpoint;

        public HealthCheckUIConventionBuilder(IEndpointConventionBuilder apiEndpoint, IEndpointConventionBuilder webhookEndpoint, IEndpointConventionBuilder resourceEndpoint)
        {
            this.apiEndpoint = apiEndpoint;
            this.webhookEndpoint = webhookEndpoint;
            this.resourceEndpoint = resourceEndpoint;
        }

        public void Add(Action<EndpointBuilder> convention)
        {
            apiEndpoint.Add(convention);
            webhookEndpoint.Add(convention);
            resourceEndpoint.Add(convention);
        }
    }

    public static class Extensions
    {
        public static IEndpointConventionBuilder MapHealthCheckUI(this IEndpointRouteBuilder builder)
        {

            var apiEndpoint = builder.Map(...);
            var webhooksEndpoint = builder.Map(...);
            var resourceEndpoint = builder.Map(...);

            return new HealthCheckUIConventionBuilder(apiEndpoint, webhooksEndpoint, resourceEndpoint);
        }
    }

This is similiar to how Blazor is adding the Hub: https://github.com/aspnet/AspNetCore/blob/2e4274cb67c049055e321c18cc9e64562da52dcf/src/Components/Server/src/Builder/ComponentEndpointConventionBuilder.cs#L11

https://github.com/aspnet/AspNetCore/blob/4a7bf75e89e3dfe7264a641ed041f50788d28d20/src/Components/Server/src/Builder/ComponentEndpointRouteBuilderExtensions.cs#L112

[CarlosLanderas]

@WolfspiritM you are right. Let us rework the internals to achieve this.

[WolfspiritM]

I tested latest version and at least the informations are now secured. Thank you! Is it on purpose that the app (resources) in itself is still available even so it's not able to pull data without authentication?

[CarlosLanderas]

Hello @WolfspiritM. I've configured internally the endpoints so you can wire authorization metadata for the healthchecks and Webhooks endpoints but protecting the UI spa resources / adding authentication mechanisms like idsrv / azure AD is out of the scope of the project at this moment.

[WolfspiritM]

@CarlosLanderas Thank you for the answer. I can understand that adding authentication to the UI is out of the scope but can you tell me what the problem is with passing the result of the MapGet's in https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/bc0f481404412015aa11aeb30709eebba5dcd9dc/src/HealthChecks.UI/Core/UIEndpointsResourceMapper.cs#L19 to the HealthChecksUIConventionBuilder aswell? That way the metadata is added to all the endpoints and the good thing is that AspNetCore will take care of authentication and authorization. Right now it works for me as it doesn't provide any critical informations anymore but some people might like to limit the endpoint to a single host completly or might want to hide the SPA endpoint aswell when not authorized to not give any informations about the technology used.

EDIT: Another problem with current implementation is that for Basic auth it works but if I really want to add IdSrv authentication then the redirect the middleware triggers will only cause the API calls to fail but the SPA in itself isn't redirected.

[CarlosLanderas]

Sorry. I explained myself about the authentication part but not about the statics. We can definitely add the resources endpoints to the convention builder. I'm gonna add it 👍

[WolfspiritM]

Thank you very much 👍

[CarlosLanderas]

@WolfspiritM, I've merged the previous pending UI pull request and right now I'm working on this. Gonna test this code but it should protect all resources:

            var resourcesEndpoints = new UIEndpointsResourceMapper(new UIEmbeddedResourcesReader(embeddedResourcesAssembly))
                .Map(builder, options);

            var apiEndpoint = builder.Map(options.ApiPath, apiDelegate)
                                .WithDisplayName("HealthChecks UI Api");

            var webhooksEndpoint = builder.Map(options.WebhookPath, webhooksDelegate)
                                .WithDisplayName("HealthChecks UI Webhooks");

            var endpointConventionBuilders = new List<IEndpointConventionBuilder>(new[] { apiEndpoint, webhooksEndpoint }.Union(resourcesEndpoints));

            return new HealthCheckUIConventionBuilder(endpointConventionBuilders);