Xabaril / AspNetCore.Diagnostics.HealthChecks

Enterprise HealthChecks for ASP.NET Core Diagnostics Package
Apache License 2.0
4.05k stars 792 forks source link

Modify Azure Service Bus Health Check to no require a connection string with management rights #727

Open paterson-deshommes opened 3 years ago

paterson-deshommes commented 3 years ago

What would you like to be added:

I noticed that in order for the health check calls AddAzureServiceBusSubscription() and AddAzureServiceBusTopic() to work properly, we need to submit a connection string with management rights. I think it would be preferable if that was not required and if the health check could still be done even if the connection string only had listen or send rights.

Why is this needed: Requiring management rights goes against the principles of least privileges I think and it forces me to decide between the convenience of using the library at the risk of giving more access that I want to my app versus only giving it the access it needs but having to write custom health check for the service bus.

Possible Solutions: For a connection string with listen policy, you could peek on the service bus and see if that works:

private async Task<Message> PeekServiceBusMessage(string connectionString)
        {
            var connectionStringBuilder = new ServiceBusConnectionStringBuilder(connectionString);
            IMessageReceiver receiver = new MessageReceiver(connectionStringBuilder);
            return await receiver.PeekAsync();
        }

For a send policy, it could be simply sending a dummy message:

try
            {
                await using (var client = new ServiceBusClient(ServiceBusConnectionString))
                {
                    ServiceBusSender sender = client.CreateSender(topic);
                    await sender.SendMessageAsync(new object());
                }
            }
            catch(Exception e)
            {
                //do something
            }

There might be more effective solutions, those are just some propositions off the top of my head.

romek-b commented 3 years ago

This is important. An application should not include a connection string with permissions to delete a queue when it need just to listen/send and check health.

ystvan commented 2 years ago

I'd love to see this resolved! 💯

ncoussemacq commented 2 years ago

I would really like this to be fixed as well.

BenjaminUL893 commented 2 years ago

One way we resolved this is by using an older version of the package (5.0.1 of AspNetCore.HealthChecks.AzureServiceBus) and then just using managed identity. The connection string looks something like:

healthChecksBuilder.
    AddAzureServiceBusTopic(
    $"Endpoint=sb://{options.Namespace}/;Authentication=ManagedIdentity;",
    options.Topic,
    name: "My Health check");

The service you're using this on needs to have the built-in Azure Service Bus Data Sender role assigned to it from the Service Bus Topic (or Queue).

SergeiBelialov commented 2 years ago

Vote up on this one

vitaly-pavluk commented 2 years ago

I bumped into this problem when I blindly upgraded health check NuGet to the latest version ... Now trying to explain to the management why my service steadily reports Unhealthy status but at the same time handles messages from the Service Bus... The security team does not allow to add Manage permission but SRE team wants me to fix tons of errors reported by the healtchecks.

SergeiBelialov commented 2 years ago

@vitaly-pavluk same thing. Our solution was:

  1. Get rid of this package.
  2. Add Microsoft.Azure.ServiceBus.Management
  3. Created our own : IHealthCheck extension, where we use ManagmentClient.TopicExistsAsync() method to check if it's healthy or not.
vitaly-pavluk commented 2 years ago

@SergeiBelialov thanks. it seems that I have to do the same... arrrggghhhhhh!!!!!! :-(

sungam3r commented 2 years ago

If someone has a solution, then PR is welcome.

brunohdossantos commented 2 years ago

I'm having the same problem, could you give more attention. As a good practice, connections should not have "Manage" permission, they should have permission, for example, only "listen" or "send" according to their purpose.

brunohdossantos commented 2 years ago

@vitaly-pavluk same thing. Our solution was:

  1. Get rid of this package.
  2. Add Microsoft.Azure.ServiceBus.Management
  3. Created our own : IHealthCheck extension, where we use ManagmentClient.TopicExistsAsync() method to check if it's healthy or not.

did it work with connection string without management permission?

brunohdossantos commented 2 years ago

@vitaly-pavluk same thing. Our solution was:

  1. Get rid of this package.
  2. Add Microsoft.Azure.ServiceBus.Management
  3. Created our own : IHealthCheck extension, where we use ManagmentClient.TopicExistsAsync() method to check if it's healthy or not.

After using some tests, note this connection method also needs "Manage" permission

mapaille commented 1 year ago

I would like this to be fixed as well.

vitaly-pavluk commented 1 year ago

Seems this issue is miss-addressed. Need to contact Microsoft so they can handle that on their side in the Service Bus. The only workaround I found - was the creation of the managed identity with a fine-grained permission set and use it.

viktoreinars commented 1 year ago

We are forced to stop using this package for health checks to our service bus. To risk handing someone a Manage claim to our service bus (if the service were compromised) for health checks is simply not an option.

Please fix this.

admalledd commented 1 year ago

Supposedly #1503 / #1504 did some of this (Specifically for Queues), is there a chance for a update to the published nuget with those included?

I also don't think the health-checks should send anything because that may impact other services when they receive said message.

sungam3r commented 1 year ago

@paterson-deshommes Could you confirm that you initial suggestion was implemented?

paterson-deshommes commented 1 year ago

@sungam3r I don't have the setup required to check this anymore. Could anyone look that up?

ghost commented 1 year ago

Seems this issue is miss-addressed. Need to contact Microsoft so they can handle that on their side in the Service Bus. The only workaround I found - was the creation of the managed identity with a fine-grained permission set and use it.

Hello @vitaly-pavluk you created a custom role for the managed identity? Could you share the permissions (Actions and Data Actions) you set? The Azure Service Bus built-in roles have either less or to much permissions, I am not sure which ones to remove or add to only guarantee Azure Service Bus Health Checks can be done.

Thanks in advance!