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

Simplify experience when using endpoint-based connection: leverage DefaultAzureCredential by default #547

Open
julealgon opened this issue Apr 16, 2024 · 5 comments

Comments

@julealgon
Copy link

The Connect(Uri, TokenCredential) API forces the consumer to always specify a token credential, which to me seems fairly unnecessary considering we have a very sane default called the DefaultAzureCredential class.

I propose one (or 2) of the following:

  1. Create a new overload to IConfiguraitonBuilder.AddAzureAppConfiguration that takes just a Uri object, and uses new DefaultAzureCredential behind the scenes.
  2. Modify the existing AzureAppConfigurationOptions.Connect(Uri, TokenCredential) method to make the tokenCredential argument optional: if it is not specified, default to new DefaultAzureCredential
  3. [Alternative to 2 if default arguments are to be avoided] Create a new overload for AzureAppConfigurationOptions.Connect(Uri, TokenCredential) that takes only the Uri argument and defaults the credential internally to new DefaultAzureCredential

I believe this would significantly streamline configuration, especially when you connect to multiple instances like we do. This is currently our implementation for context:

builder.Configuration
    .AddAzureAppConfiguration(c =>
        c.Connect(
            endpoint: builder.Configuration.GetValue<Uri>("Azure:AppConfiguration:Common:Endpoint"),
            credential: new DefaultAzureCredential()))
    .AddAzureAppConfiguration(c =>
        c.Connect(
            endpoint: builder.Configuration.GetValue<Uri>("Azure:AppConfiguration:Endpoint"),
            credential: new DefaultAzureCredential()));

And this is how it would look like with option 1 above:

builder.Configuration
    .AddAzureAppConfiguration(builder.Configuration.GetValue<Uri>("Azure:AppConfiguration:Common:Endpoint")
    .AddAzureAppConfiguration(builder.Configuration.GetValue<Uri>("Azure:AppConfiguration:Endpoint");

PS: The reason we fetch the endpoint from configuration itself is because we have dedicated instances of Azure AppConfig for each of our environments.

I'm aware this would add a new dependency to Azure.Identity on the package (since every single implementation of TokenCredential comes from there) but to me that seems fully justifiable and I'd even make the argument that it would simplify this interaction even more to the consumer as he doesn't need to be aware of a separate package that needs to be introduced to even be able to call these endpoint-based overloads.

If the team is somehow concerned over this new dependency addition, then I'd propose this new overload exist as part of a separate Microsoft.Extensions.Configuration.AzureAppConfiguration.Identity package that references both Microsoft.Extensions.Configuration.AzureAppConfiguration and Azure.Identity. I personally think that would be overkill, however.

This expands:

And apparently is somewhat of a "rollback" of this?

Looks like at the time there were some nasty dependencies on NewtonsoftJson and System.Interactive.Async that don't exist anymore in Azure.Identity.

@amerjusupovic
Copy link
Member

Hi @julealgon, the only thing I'm worried about here is the dependency on Azure.Identity, otherwise I think it kinda makes sense to use DefaultAzureCredential since that's what our examples will always show anyway. Recently I've noticed that Azure.Identity often has a new vulnerability that requires releasing a new version, and I don't know if we'd want to be consistently releasing new versions of this repo to update that dependency. I don't really know the nature of those vulnerabilities, but if it's marked as moderate severity it sounds like it'll require an update.

Introducing a separate package does seem like kind of a big deal as well if it's just for convenience. That might make releasing new versions easier, but we'd need to actively track the Azure.Identity package for vulnerabilities, and I'm not sure that's worth it. Let me know if you think differently.

@julealgon
Copy link
Author

Thanks @amerjusupovic .

I will say... we just hit another hurdle because of this due to key vault references... now we are also forced to add this (in my opinion incredibly unnecessary):

.ConfigureKeyVault(kv => kv.SetCredential(new DefaultAzureCredential());

I just don't see why things like this are not the default setting. Not only we have to specify it explicitly, but it also looks incredibly redundant considering we just specified that credential for the appconfig instance itself on the Connect method.

This API surface is not as user friendly as it could be ATM.

@samsadsam
Copy link
Contributor

I kind of agree with @julealgon.

@zhenlan @drago-draganov what's your take on this?

@samsadsam samsadsam moved this from Todo to Backlog in AppConfiguration-DotnetProvider Oct 23, 2024
@zhenlan
Copy link
Contributor

zhenlan commented Nov 3, 2024

Here are my thoughts.

  • It's often a design preference. Some prefer the "just works" magic, while others dislike it because it makes debugging difficult when things go wrong. Although the DefaultAzureCredential simplifies things to some extent, it doesn't tell the whole story. Users still need to understand which credential is used in each environment and ensure the proper role assignment. For more details, see my discussion on authentication with token credentials. Therefore, our design favors being explicit rather than relying on magic.

  • While the DefaultAzureCredential offers certain conveniences, it comes with costs such as debugging challenges, performance overhead, and unpredictable behavior. To learn more, see the Usage guidance for DefaultAzureCredential, which recommends using more specific token credentials.

  • The Azure.Identity library (where the DefaultAzureCredential resides) inherently carries higher security risks due to its nature. Depending on DefaultAzureCredential means we must explicitly depend on the library, which forces a specific version of Azure.Identity on our customers, even if they don't use Entra ID authentication at all.

@julealgon
Copy link
Author

@zhenlan

Here are my thoughts.

  • It's often a design preference. Some prefer the "just works" magic, while others dislike it because it makes debugging difficult when things go wrong. Although the DefaultAzureCredential simplifies things to some extent, it doesn't tell the whole story. Users still need to understand which credential is used in each environment and ensure the proper role assignment. For more details, see my discussion on authentication with token credentials. Therefore, our design favors being explicit rather than relying on magic.

If you think it should always be explicit like that, then I believe the class should be renamed to something else other than DefaultAzureCredential. Having Default in the name of a concrete implemetation of an interface, to me, signals it is the concrete class used by default, and that it can be replaced with something else.

I don't consider this "magic" behavior. It is just a sensible default. If it could not be overridden, then I'd agree with you.

  • While the DefaultAzureCredential offers certain conveniences, it comes with costs such as debugging challenges, performance overhead, and unpredictable behavior. To learn more, see the Usage guidance for DefaultAzureCredential, which recommends using more specific token credentials.

Some of these issues seem like problems on the SDK side? Debugging challenges and performance overhead should be able to minimized at the library side, surely? I also hope the perf overhead is only on the first access, as the information is cached. If that's not the case I'd be a lot more worried about it and would like to know.

Regarding "unpredictable behavior", to me, that's a super weird way of saying "being configurable using environment variables", which to us is an obviously beneficial feature. Without support for environment variables, I have no safe way to provide AZURE_CLIENT_SECRET to a bunch of legacy on-premise applications. The way the article sounds make this look like it is some sort of liability. I strongly disagree with that perspective. If that was the case, anything "configuration-driven" would be just as problematic since configuration can be loaded from sources external to the application.

I'm a bit surprised such absurd argument (IMHO) is in the official docs.

  • The Azure.Identity library (where the DefaultAzureCredential resides) inherently carries higher security risks due to its nature. Depending on DefaultAzureCredential means we must explicitly depend on the library, which forces a specific version of Azure.Identity on our customers, even if they don't use Entra ID authentication at all.

You could easily create a separate package that contained the base AzureAppConfig library plus Azure.Identity and have it add extension methods there to configure the provider in a more sensible manner for those using Azure credentials though. For those who don't need that, they could keep just referencing the base package.

Many Microsoft libraries are built this way, so again it surprises me the team here sees that as some sort of big challenge.


In any case, I do hope the team changes its mind eventually. I'd like to avoid having to maintain custom extension methods/private nugets to streamline the API. I wish I didn't have to.

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

No branches or pull requests

4 participants