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

chore(deps): deprecate arcus.security references in service-bus projects #471

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

stijnmoreels
Copy link
Member

@stijnmoreels stijnmoreels commented Feb 11, 2025

  • Deprecate all references to Arcus.Security types in the Service bus-related projects with a deprecation message on what to do instead.
  • Remove all mentioning of the Arcus secret store in the Service bus-related documentation, and instead promote managed identity.

This is part of the excerise described in #470 where the Messaging repo becomes more lightweight and less dependent on other packages.

@stijnmoreels stijnmoreels added dependencies Pull requests that update a dependency file integration:service-bus All issues concerning integration with Azure Service Bus labels Feb 11, 2025
Copy link

netlify bot commented Feb 11, 2025

Deploy Preview for arcus-messaging canceled.

Name Link
🔨 Latest commit 9539465
🔍 Latest deploy log https://app.netlify.com/sites/arcus-messaging/deploys/67bd72ce3897a50008695f30

@fgheysels
Copy link
Member

I think we should not only support ManagedIdentity to connect to ServiceBus.
While we can remove the dependency to Arcus.Security, I think we should still have a way to connect to ServiceBus by specifying a clientid / clientsecret for instance. But, instead of passing in a secretname and use that secretname to retrieve the secret from KeyVault using Arcus.Security, we could pass in the secret directly.
It's then the responsibility of the client application to retrieve the secret from a secure location and use it.

@stijnmoreels
Copy link
Member Author

I think we should not only support ManagedIdentity to connect to ServiceBus. While we can remove the dependency to Arcus.Security, I think we should still have a way to connect to ServiceBus by specifying a clientid / clientsecret for instance. But, instead of passing in a secretname and use that secretname to retrieve the secret from KeyVault using Arcus.Security, we could pass in the secret directly. It's then the responsibility of the client application to retrieve the secret from a secure location and use it.

That's still kinda possible as we still support the IConfiguration overload, where people can register their own source/authentication and retrieve it there.

@fgheysels
Copy link
Member

I think we should not only support ManagedIdentity to connect to ServiceBus. While we can remove the dependency to Arcus.Security, I think we should still have a way to connect to ServiceBus by specifying a clientid / clientsecret for instance. But, instead of passing in a secretname and use that secretname to retrieve the secret from KeyVault using Arcus.Security, we could pass in the secret directly. It's then the responsibility of the client application to retrieve the secret from a secure location and use it.

That's still kinda possible as we still support the IConfiguration overload, where people can register their own source/authentication and retrieve it there.

I'd have to take a look at what you mean exactly.
My idea would just be to provide a method that allows you to specify the clientid / clientsecret itself. Arcus should not be responsible for retrieving that information. The program / component that hosts Arcus.Messaging should be responsible.

@stijnmoreels
Copy link
Member Author

I'd have to take a look at what you mean exactly.
My idea would just be to provide a method that allows you to specify the clientid / clientsecret itself. Arcus should not be responsible for retrieving that information. The program / component that hosts Arcus.Messaging should be responsible.

Currently supported, even after this PR:

Host.CreateDefaultBuilder()
    .ConfigureAppConfiguration(config => config.AddAzureKeyVault(new Uri(...), new ClientSecretCredential(...))
    .ConfigureServices(services =>
    {
        services.AddServiceBusTopicMessagePump("<subscription>", (IConfiguration config) =>
        {
            return config["Topic:ConnectionString:From:IConfig:Via:KeyVault");
        });
    });

@fgheysels
Copy link
Member

I'd have to take a look at what you mean exactly.
My idea would just be to provide a method that allows you to specify the clientid / clientsecret itself. Arcus should not be responsible for retrieving that information. The program / component that hosts Arcus.Messaging should be responsible.

Currently supported, even after this PR:

Host.CreateDefaultBuilder()
    .ConfigureAppConfiguration(config => config.AddAzureKeyVault(new Uri(...), new ClientSecretCredential(...))
    .ConfigureServices(services =>
    {
        services.AddServiceBusTopicMessagePump("<subscription>", (IConfiguration config) =>
        {
            return config["Topic:ConnectionString:From:IConfig:Via:KeyVault");
        });
    });

Yeah, but it's a pity you have to use it like that if your program that is using Arcus.Messaging is also using Arcus.Security.
In such case, I think it would be better if we can directly pass in the clientid/clientsecret.
I'l have to peek into the code to see if I can provide an example of what I mean.

@stijnmoreels
Copy link
Member Author

stijnmoreels commented Feb 18, 2025

Yeah, but it's a pity you have to use it like that if your program that is using Arcus.Messaging is also using Arcus.Security. In such case, I think it would be better if we can directly pass in the clientid/clientsecret. I'l have to peek into the code to see if I can provide an example of what I mean.

Think that is a bit the purpose, that people see that they need to migrate to something else.
Happy to discuss this separately.

@fgheysels
Copy link
Member

I'd have to take a look at what you mean exactly.
My idea would just be to provide a method that allows you to specify the clientid / clientsecret itself. Arcus should not be responsible for retrieving that information. The program / component that hosts Arcus.Messaging should be responsible.

Currently supported, even after this PR:

Host.CreateDefaultBuilder()
    .ConfigureAppConfiguration(config => config.AddAzureKeyVault(new Uri(...), new ClientSecretCredential(...))
    .ConfigureServices(services =>
    {
        services.AddServiceBusTopicMessagePump("<subscription>", (IConfiguration config) =>
        {
            return config["Topic:ConnectionString:From:IConfig:Via:KeyVault");
        });
    });

True.
But, then you're forced to store a connectionstring (secret) in your configuration. One of the reasons we have Arcus.Security is to make a distinction between secrets and configuration.
When you want to go that route, you'll need to add your secret-store (KeyVault for instance) as a Configuration-source (which is possible, but do we want that ?). Another solution is to use KeyVault-references in your Configuration (which is also a valid way of working but then again, no explicit distinction between secrets & configuration).

I do understand that you want to remove the dependency to Arcus.Security though.
Therefore, as an alternative for teams that use Arcus.Messaging and Arcus.Security, I would provide an option to pass in the connection-string directly.
It's then the responsability of the program that uses those libraries to retrieve the connectionstring from the secret-store and pass it to Arcus.Messaging. Something like this:

string connectionstring = await secretProvider.GetRawSecretAsync(mySecretName);

services.AddServiceBusQueueMessagePump(connectionString);

string entityName,
string subscriptionName,
ServiceBusEntityType serviceBusEntity,
Func<IConfiguration, string> getConnectionStringFromConfigurationFunc,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of having to retrieve the connctionstring from config.
Why not just pass in the connectionstring as a string ? Its the responsibility of the user to retrieve it from any location he wants.

@@ -29,6 +29,7 @@ public static class AzureClientFactoryBuilderExtensions
/// <exception cref="ArgumentException">Thrown when the <paramref name="connectionStringSecretName"/> is blank.</exception>
/// <exception cref="InvalidOperationException">Thrown when the Arcus secret store is not registered.</exception>
/// <exception cref="SecretNotFoundException">Thrown when no Azure EventHubs connection string secret was found in the Arcus secret store.</exception>
[Obsolete("Will be removed in v3.0, please use Microsoft's built-in Azure SDK clients to register a " + nameof(ServiceBusClient) + " to remove the " + nameof(ISecretProvider) + " requirement")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can make the entire class obsolete ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably in time, but the scope of this PR is only to deprecate Arcus.Security references.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my mistake, thought that this was another extensions class, yes we can.

@stijnmoreels
Copy link
Member Author

stijnmoreels commented Feb 21, 2025

Best to discus this separately: 'What to do with projects that use Arcus.Security in combination with Arcus.Messaging.’

What I think about:

  1. Arcus.Security is valid and ambitious way of separating config from secrets, but it is yet another repository to maintain, which cannot be done in the current way of working;
  2. Arcus.Messaging provides still added-value and can definitely have a future and support when it becomes lightweight enough to maintain further. To do that, we need to reduce the dependencies (Arcus.Security, Arcus.Observabiltity, ApplicationInsights, AzureFunctions and Serilog come to mind).
  3. Figuring out what to expose as functionality is critical, as this has to be modular enough. (The thing you proposed of having the connection string directly, would mean yet an extra extension for an already too-big extension set for registering message pumps. Also: retrieving a secret store at that point isn’t really possible in a safe way, I believe, as the pump is registered at the same time as the secret store: through the services).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file integration:service-bus All issues concerning integration with Azure Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants