Change Request: Support components of connection string not present in the portal

ramya-rao-a commented 3 years ago

The Basics

Which design guideline is affected?

Use of connection strings in client constructors

Which target languages are affected?

All

Describe the change:

A connection string is made of key/value pairs. Current understanding is that only those keys that are present in the connection string in the Azure portal should be supported by client constructors.

Relevant parts from the general guidelines for auth:

DO NOT support constructing a service client with a connection string unless such connection string is available within tooling (for copy/paste operations).

Relevant parts from the .Net guidelines for auth

YOU MAY offer a way to create credentials from a connection string only if the service offers a connection string via the Azure portal.

Don’t ask users to compose connection strings manually if they aren’t available through the Azure portal. Connection strings are immutable. It’s impossible for an application to roll over credentials when using connection strings.

The change being requested here is to have the client constructor support connection string with keys that are not present in the portal and would require a user to either hand-write the connection string or use a helper to build one.

The request is coming from Service Bus and Event Hubs teams where their existing libraries support the below components in the connection string:

SharedAccessSignature
TransportType (WebSockets or AMQP via port 5671)
Indicator of whether Managed Identity should be used for auth

Reasoning

Benefits of such kind of support is that a user needs to only change the connection string in the config of their choice and not change any code when they want to move between environments which have different requirements. Examples:

In my dev environment, I'd like to use the connection string I copied from the portal, but my test environment makes use of short lived SharedAccessSignature provided by my test framework
In certain production environments, the port 5671 is not available. So, I'd like to use the TransportType corresponding Web Sockets
Some of the queues against which my code runs do not have AAD set up, but others do. So, I'd like to use the connection string from the portal in the former and Managed Identity in the latter

Please provide any additional detail or reasoning for the request.

The need to support SharedAccessSignature regardless of whether as part of a connection string or as another constructor overload is important in user applications that use a Security Token Service. Here is a custom solution for STS using Azure functions by @clemensv that is being advocated for users working with Service Bus. Another scenario is where we want to provide short lived access on purpose or prevent the client from knowing the key. See Authenticating Event Hubs publishers with SAS

Interested parties for this discussion:

Service Bus representatives: @shankarsama, @axisc, @yvgopal
Event Hubs representatives: @sjkwak, @serkantkaraca, @JamesBirdsall
Messaging Architect: @clemensv
Messaging crew on the SDK team across .Net, JS, Java and Python: @JoshLove-msft, @MiYanni, @jsquire, @richardpark-msft, @HarshaNalluru, @chradek, @conniey, @hemanttanwar, @srnagar, @KieranBrantnerMagee, @YijunXieMS, @yunhaoling

cc @KrzysztofCwalina who was present in the offline discussion on this topic with @clemensv cc @JeffreyRichter who lead the initial discussions in the SDK team on the matter of connection string support

richardpark-msft commented 3 years ago

Is the implication here that client libraries should also not validate connection strings? We have a guideline that basically says to prefer service-side validation rather than client-side validation - in ways this feels similar.

ramya-rao-a commented 3 years ago

Is the implication here that client libraries should also not validate connection strings? We have a guideline that basically says to prefer service-side validation rather than client-side validation - in ways this feels similar.

I don't this so. From my understanding of the services we have worked with so far, the connection string is never passed as is to the service. It is always parsed, different parts used appropriately by the client to make a request to the service. If parsing is involved, then some kind of validation would also be involved.

The guideline for preferring service side validation rather than client side validation is for cases where the client can make the connection to the service successfully without doing any validation on its end. A hypothetical example could be restrictions on the length of the queue name.

The request here would not remove the need for validation, because the client is always responsible to parse the connection string, extract relevant parts and use that appropriately.

In case of Service Bus and Event Hubs, the client would

validate that TransportType is one of the supported types (currently just two), because this information is not passed to the service, but instead used to decide how to set up the connection
not validate SharedAccessSignature other than it should be present and is not an empty string. The service will throw appropriate error if an invalid value is passed, there is no way for the client to do any other validation

JeffreyRichter commented 3 years ago

It does seem like this request is only because Track 1 supported this and so the "feature" went away in Track 2. So, if we enabled this, I'd only do it for SB/EH in languages where a Track 1 existed and not for other services/languages.

I understand the desire to set things in 1 place (like an environment variable) for different testing scenarios. But I still think that this convenience does not put the customer on the right long term path. Why can't the "other stuff" be in another environment variable or config file and the customer code parse it (typically languages offer helper functions for this) and then pass them in when creating the client? This way if something goes wrong (env var not found, file can't be opened, etc.), the customer stands a chance of debugging the issue and fixing it themselves.

So what are the problems with connections strings... Different services have different connection string syntaxes. Connection strings are not just a set of key/value pairs. The case of the keys/values matter, the order may matter (it does for Storage), what happens if a key appears more than once? What happens if conflicting keys are specified? How does he customer debug a typo in the key or value? What if whitespace appears in the string around equal signs? Is the whitespace relevant or not? Are multiple key/values separated by commas, semicolons, or something else? Can any of these delimiters also be meaningful for a value and therefor not a delimiter?

The end result is that there is a lot of magic that happens with connection strings, lots of potential failures and difficult for a customer to know what happened, diagnose it, and fix it themselves.

In addition, many of our client types allow changing (rolling over/refreshing) credentials while multiple threads are using the same client. We allow this without stopping the customer's app using a client object because we can update the credentials in a thread safe/atomic way. Since connection strings frequently have the endpoint and credentials in a single string, getting a new connection string means tearing the whole client down and reconstructing it even if only the credential has changed. This is what puts customers on a bad path for building an app that runs 24x7. Connection strings should not combine a bunch of unrelated things together which are treated as a single atomic unit - it's convenient to get started (except for all the potential bugs I mentioned earlier) but inconvenient for building a long-running app.

Now, for AMQP which is a connection-full protocol maybe it makes more sense to tear down the entire client object and reconstruct it because the endpoint and the credentials are tied together. But for HTTP, the endpoint (URL) and the credentials are not. So again, if we add support for more connection string key/values, then I'd only do this for SB/EH when using AMQP.

JeffreyRichter commented 3 years ago

Is the implication here that client libraries should also not validate connection strings? We have a guideline that basically says to prefer service-side validation rather than client-side validation - in ways this feels similar.

@richardpark-msft : You can't validate connection strings on the server because they are never sent to the server. They are purely for convenience on the client side to create a client object.

JoshLove-msft commented 3 years ago

It does seem like this request is only because Track 1 supported this and so the "feature" went away in Track 2. So, if we enabled this, I'd only do it for SB/EH in languages where a Track 1 existed and not for other services/languages.

I'll just note that Storage Track 1 had a similar concept with the CloudStorageAccount type that could be used to add additional key/value pairs into a connection string. This was not carried forward to Track 2. /cc @tg-msft

JeffreyRichter commented 3 years ago

Service Team: have you ever had to help customers diagnose issues that ended up being related to connection strings?

ramya-rao-a commented 3 years ago

Copying an update I made to the issue description to the benefit of those who have already read through this issue:

The need to support SharedAccessSignature regardless of whether as part of a connection string or as another constructor overload is important in user applications that use a Security Token Service. Here is a custom solution for STS using Azure functions by @clemensv that is being advocated for users working with Service Bus. Another scenario is where we want to provide short lived access on purpose or want to prevent the client from knowing the key. See Authenticating Event Hubs publishers with SAS

KieranBrantnerMagee commented 3 years ago

Summarizing some points from a brief discussion that was had on this topic.

Our primary concern is the non-happy-path-nature of connstr being the properties bag. This is, recalling back to my own experiences as a dumb user, a great avenue to leaving a dev headscratching and feeling unsure about "did I edit this right? format it right? right keyword? right quoting? is this error something I caused?"

We can understand the advantage of it giving a "one-stop-shop" for unified configuration; and if this is a hard ask it's not the end of the world to support it. However, I'd lean towards an approach that tries to guardrail "most users" to the individually parameterized approach. (e.g. with overloads in kwargs or your options bag, or what have you, ala Transport=Foo) to try and heavily discourage users to fall into the riskier and less clear-cut string-munging approach.

(An interesting point came up that while connstrs were lingua-franca for e.g. DBs, the fact that there exist ecosystems of tooling to build them is a strong indicator of pitfalls-requiring-guardrails in that flow.)

richardpark-msft commented 3 years ago

What @ramya-rao-a suggests above makes sense to me - expanding the connection string to incorporate the specific attributes she's mentioned above make sense. @KieranBrantnerMagee 's concern is also valid - however users come to this there has to be something that gives them a way to compose this string, probably documentation.

My experience has been that the connection string is treated as an opaque value (I'm not editing it in an ad-hoc manner). This means that supporting a simple "copy/paste a single value" was a decent workflow. We did not edit it - it was just replaced wholesale.

For instance, in Azure Functions, we pointed to storage accounts in production with a connection string and development also used a connection string. Sometimes that connection string would point to an actual storage account but you could also use the emulator by just making your connection string "UseDevelopmentStorage=true". So this model was sufficient all the way up to production - no code changes required.

Now a further question, since is meant to apply broadly to the SDK - are we possibly creating a parallel path to Azure Identity? Would it make sense to consider having a "DefaultServiceBusCredential" that an app could incorporate that could handle several fallbacks, including taking a connection string? (This discussion might be out of scope for this particular guideline change, however).

ramya-rao-a commented 3 years ago

cc @schaabs for the idea of having a flag in the connection string that essentially says "use default azure credential"

schaabs commented 3 years ago

cc @schaabs for the idea of having a flag in the connection string that essentially says "use default azure credential"

I'm not sure I see the benefit to having this information in the connection string. Why would a user specify this in the connection string instead of calling the constructor overload which takes a TokenCredential instance?

richardpark-msft commented 3 years ago

cc @schaabs for the idea of having a flag in the connection string that essentially says "use default azure credential"

I'm not sure I see the benefit to having this information in the connection string. Why would a user specify this in the connection string instead of calling the constructor overload which takes a TokenCredential instance?

It comes down to whether users are expected to be able to use connection strings from beginning to end and not require code changes at all.

Similar to how in SQL Server connection strings you can swap between using a username and password to adding in Integrated Security=true; which will default to the "environment's" credentials.

schaabs commented 3 years ago

It comes down to whether users are expected to be able to use connection strings from beginning to end and not require code changes at all.

First, I would say that using a connection string with a SAS key or a Shared Key, is not interchangeable with the DefaultAzureCredential. When authenticating with AAD, a user needs to grant access to the account they are using, so simply pasting in a connection string from a sample will not work. Secondly, by controlling this behavior with a section of the connection string such as UseDefaultAzureCredential=true it seems like you're just moving the code into the connection string.

All that said, I'm not vehemently against the idea, I just don't see that much benefit.

richardpark-msft commented 3 years ago

[ minor edit - as has been pointed out to me DefaultAzureCredential is the wrong credential. The scenario I'm pointing out makes more sense with ManagedIdentityCredential or potentially EnvironmentCredential ]

@schaabs, I think at this point I'm dragging this part of the issue beyond what @ramya-rao-a is proposing so I'll just add in some more context here for the side point and try not to distract too much.

It comes down to whether users are expected to be able to use connection strings from beginning to end and not require code changes at all.

First, I would say that using a connection string with a SAS key or a Shared Key, is not interchangeable with the DefaultAzureCredential. When authenticating with AAD, a user needs to grant access to the account they are using, so simply pasting in a connection string from a sample will not work.

My thought is that a user could have a scenario like this:

Env: development. Pure connection string (using a SAS key or similar) Env: production. Connection string, but this time indicates "use environment's identity". Internally we then instantiate something like ManagedIdentityCredential (or equivalent) for picking up the environment level credentials.

The user, in all environments, does need to do some background setup (SAS key being easiest) but that's independent of the code. Ie, these are deployment concerns.

The key here is that the user's code does not change - they're always just creating the client and just dumbly passing the connection string and internally we use the connection string to pick how we construct the credential, which might include using Azure Identity.

Secondly, by controlling this behavior with a section of the connection string such as UseDefaultAzureCredential=true it seems like you're just moving the code into the connection string.

Yes, totally. I am not suggesting this is the right case but it seems like it'd be a logical progression and I just wanted to include it in the discussion.

All that said, I'm not vehemently against the idea, I just don't see that much benefit.

It definitely might be going too far down the road. It's just where my mind went when I thought about the connection string being less tied to the portal connection string and more a "serialization" entity to specify authentication.

YijunXieMS commented 3 years ago

Some Azure services have Spring integration, which uses plain text to configure almost everything. For instance, Storage is configured like the following. The sample tells users to generate a credential file by using Azure Command Line and configure the credential file path. With the Track2 library, this process can be better with the different choices in azure identity. Azure VM users can use MSI. They don't need to generate the credential file.

spring.cloud.azure.credential-file-path=my.azureauth
spring.cloud.azure.resource-group=wingtiptoysresources
spring.cloud.azure.region=westUS
spring.cloud.azure.storage.account=wingtiptoysstorage
blob=azure-blob://containerName/blobName

ServiceBus' Spring integration uses JMS with configuration like:

spring.jms.servicebus.connection-string=<ServiceBusNamespaceConnectionString>
spring.jms.servicebus.topic-client-id=<ServiceBusTopicClientId>
spring.jms.servicebus.idle-timeout=<IdleTimeout>

A question is whether we allow users to use an "all-around" single connection string, or separate strings for different parts of the connection configuration. Many products allow both a single connection string and separate properties. A JDBC connection string (for mysql, sql server, etc) can be a single string, including the credentials. An example is jdbc:sqlserver://localhost;instanceName=instance1;integratedSecurity=true;<more properties as required>; It also allows users to use a combination of connection string and separate properties via API. This flexibility can serve different scenarios. I remember when I was an Enterprise system developer, we liked to use a JDBC connection string that doesn't have username and password but everything else. Only the admin knew production password so we separated it from connection string.

Many messaging brokers also allow a connection string to have everything. An example is

amqp://myhost?heartbeat=10&connection_timeout=10000

KrzysztofCwalina commented 3 years ago

I like the tweak @JeffreyRichter suggested to scope this new guideline to AMQP services. Otherwise the proposal looks good.

ramya-rao-a commented 3 years ago

Thanks @KrzysztofCwalina

Pinging architects for Python, Java and TS @johanste, @JonathanGiles, @bterlson, This ok with you?

JonathanGiles commented 3 years ago

I agree with keep scope reduced until necessary. I also encourage good documentation about what is supported in the connection string for each client, and order of precedence between configuring in the connection string versus the traditional way of setting the same client configuration. Other than that, I approve.

tg-msft commented 3 years ago

I'll register an objection, at least. This is the sort of thing that becomes such a slippery slope and we shouldn't do it casually. Do we allow DefaultAzureCredential settings today? What about RetryPolicy settings tomorrow? What happens when there's a naming conflict because two different things have a Timeout property with different meanings? Do we invent a bunch of rules to disambiguate names like RetryPolicy.Timeout=20s;TokenCredentialOptions.Timeout=10s;"? What happens across languages where Azure.Core pipelines differ and a C# connection string doesn't make sense with a Java client? How much work are we planning to do for the documentation, testing, and maintenance of synthetic client-side connection string constructs across all of our supported languages?

We allowed this in Track 1 with a really subpar "config is just key/value pairs" mentality. It was wired up such that you could use a connection string, a JSON object, or an XML file so long as all of it was turned into IDictionary<string, string>. Customers never really knew what was supported for a given service and it was painful. I prefer the current guideline of only allow it for copy/pasting from the Portal. You're just three clicks away from a working sample connection string instead of a lengthy ref doc page to go read.

And if there's really a need for Track 1 connection strings to smooth the upgrade path, I'd prefer a static FooClient.FromLegacyConnectionString factory method so it's clear you're playing in a weird space with limited support. It'll also help set customer expectation around connection strings of arbitrary complexity in other client libraries.

jsquire commented 3 years ago

I'll register an objection, at least. This is the sort of thing that becomes such a slippery slope and we shouldn't do it casually. Do we allow DefaultAzureCredential settings today? What about RetryPolicy settings tomorrow?

@tg-msft : I wanted to clarify if you're objecting to a SharedAccessSignature which has a fairly limited scope and well-defined use-case or reacting to Richard's thought about extending the connection string as a DSL.

Personally, I would prefer to allow a SharedAccessSignature given that there is a defined set of use cases where you don't want semi-trusted parties to have access to the shared key information and where the management of AAD credentials would be a burden for short-term access. While you could make the case that having a custom token credential would offer an advantage because of the ability to extend the SAS, I believe that the typical use is short-lived and this isn't likely to add much value and doesn't offset the added complexity of needing a new type and another set of constructor overloads.

That said, I strongly agree with the sentiment of not arbitrarily extending connection strings as a custom grammar nor without a strong use case to back up the discussion.

johanste commented 3 years ago

+1 on keeping the scope as small as possible.

bterlson commented 3 years ago

Overall, I share Jeff's concerns with regard to connection strings being a sticky failure pit, but I am also not opposed to using connection strings for more things with strong user-based rationale and we do so very rarely.

I agree with @JeffreyRichter's "AMQP services only" limitation, but does it generalize? It could mean we're generally ok adding more configuration keys to connection strings when the settings cannot change over the lifetime of a client? But not exactly because e.g. if I declare retryPolicy can't change, can I put it in connection strings now?

Getting back to @ramya-rao-a's OP, it sounds like we don't have consensus on simply dropping those guidelines but we want to carve out an exception. Would be good to get an idea of what the new guidelines should look like before closing out this issue.

tg-msft commented 3 years ago

@jsquire - If it's not in the Portal, I'd prefer we don't support it via our .ctors and use something like a FooClient.FromLegacyConnectionString static helper. It makes the exception carving easier. (But I'm not going to stand in the way if all the architects are in agreement here.)

What's the scenario for new Track 2 usage today? Somebody just wants to stuff connection information into a single env var?

I'd rather create a FooConnectionOptions object to make it super clear to customers exactly what's supported:

public class FooConnectionOptions
{
    public Uri Endpoint { get; }
    public Credential Credential { get; }
    // ... other properties ...

    // One .ctor per FooClient ctor?
    public FooConnectionOptions(...) { ... }

    // Create a client from the connection
    public FooClient CreateFooClient() { ... }

    // Translate to/from connection strings
    public string ToConnectionString() { ... }  // => "Endpoint=...; Credential=...;"
    public static FooConnectionOptions FromConnectionString(string connectionString) { ... }
}

This would be a new pattern though and require some Arch Board discussion to make sure it worked well across services, etc.

ramya-rao-a commented 3 years ago

What's the scenario for new Track 2 usage today? Somebody just wants to stuff connection information into a single env var?

@tg-msft,

The scenario is what the service team has advocated the customers to do over the past several years on how to configure all relevant options around a "connection" via a connection string. This way, there is a single entity to tweak when moving from one environment to another where the connection needs may be different without doing any code changes to their application. Whether this connection string lives in a env var or a config file is an implementation detail on the customer's part.

To avoid the pitfalls of handwritten connection string, the service team has provided a ServiceBusConnectionStringBuilder in Track 1 which is similar to your FooConnectionOptions. This was meant as a helper method that one could run in a console app to generate the connection string and then use it in their config files.

jsquire commented 3 years ago

@tg-msft: In addition to Ramya's response, there's some additional context around the "publisher" scenario available in #11910.

SeanFeldman commented 3 years ago

Good discussion. I'll chime in on behalf of my customers. And specifically, address this statement:

Why would a user specify this in the connection string instead of calling the constructor overload which takes a TokenCredential instance

Not always customers have access to the code directly. Sometimes they use components (3rd party) and passing configuration, such as to use the managed identity, via connection string that has to be provided to connect to the service bus service.

The following comment by @ramya-rao-a is spot-on:

The scenario is what the service team has advocated the customers to do over the past several years on how to configure all relevant options around a "connection" via a connection string. This way, there is a single entity to tweak when moving from one environment to another where the connection needs may be different without doing any code changes to their application. Whether this connection string lives in a env var or a config file is an implementation detail on the customer's part.

To avoid the pitfalls of handwritten connection string, the service team has provided a ServiceBusConnectionStringBuilder in Track 1 which is similar to your FooConnectionOptions. This was meant as a helper method that one could run in a console app to generate the connection string and then use it in their config files.

JeffreyRichter commented 3 years ago

If an app wants to offer config via some mechanism, it can. I do not believe that supporting this kind of config is the responsibility of our client libraries. And, in fact, you could make the reverse argument, that just because an administrator could configure the endpoint doesn't mean they should be able to change the port or the auth mechanism.

jsquire commented 3 years ago

Hi @SeanFeldman. I would tend to look at authentication and messaging as two distinct bounded contexts. The messaging context needs understand how to authorize its own use, but shouldn't be crossing into another context to perform authentication. I'd position concepts like a SAS token or shared key as authorization specific to messaging which, in my opinion, makes messaging solely responsible for them. By extension, that makes them reasonable candidates for the connection string.

To look at it in a more concrete way, if the Service Bus library starts to perform authorization, it introduces a dependency on the Identity library (and inherits all of its dependencies). That puts us squarely back into the position where many of the earlier generation libraries were - potentially having version conflicts between the packages that our library needs and those that the application would prefer. A good example of this is the System.IdentityModel.Tokens.Jwt library which is used extensively across the track one space and for which we see a fair number of reports around the friction that it causes.

To echo the point that Jeff made, I see the third party component that you describe as being responsible for owning the authentication and messaging contexts and the data flow between them. If the component wished to offer the ability to use multiple paths for authN/authZ using configuration-based approach, it would be the responsible party for extending the connection string grammar or define it's own to meet the application scenarios which the component was designed to address.

Having spent a large portion of my career in the enterprise development space, I can't think of an environment validated in the manner that you're describing where changing the core approach to auth, such as going from shared key to MSI, would not trigger a security audit and review similar to that required for a new deployment.

jsquire commented 3 years ago

That all said, I'd appreciate your help in better understanding the specific scenarios that you've got in mind.

Assuming that authentication is not a part of messaging libraries and that a variant of the ConnectionStringBuilder is introduced into the library, allowing applications to parse/create connection strings agnostic of the specifics, what gaps would you see?

ramya-rao-a commented 3 years ago

Thanks for the input everyone

I believe we are in agreement in supporting the SharedAccessSignature in the connection string and that having a helper/builder for building such a connection string is required to avoid mistakes due to hand-written efforts. We will go ahead with the support for SharedAccessSignature in connection string for Event Hubs and Service Bus.

The question to the guideline writers though is this: Shouldnt any Azure service that supports SAS and connection strings be able to have clients that support taking SAS in the connection string? Why is the need to have short lived restricted access restricted to just Service Bus and Event Hubs?

if the Service Bus library starts to perform authorization, it introduces a dependency on the Identity library (and inherits all of its dependencies). That puts us squarely back into the position where many of the earlier generation libraries were - potentially having version conflicts between the packages that our library needs and those that the application would prefer. A good example of this is the System.IdentityModel.Tokens.Jwt library which is used extensively across the track one space and for which we see a fair number of reports around the friction that it causes

@jsquire Thanks for bringing up this point of client libraries needing to depend on Identity library if we were to support the choice to use Managed Identity just by looking at the connection string rather than have the user provide a credential from the Identity library. Avoiding the dependency on the Identity library from inside the individual client libraries was one of the very intentional design decisions that was made early on in the guidelines.

Can we perhaps consider a constructor overload that takes both the connection string and a TokenCredential? The absence of the any SAS related details in the connection string can then be taken as an indicator to use the provided credential?

This will serve the need to configure SAS vs Managed Identity via a connection string without changing code and avoids the dependency on the Identity library from inside the client

Taking EventHubProducerClient as an example

Before:

public EventHubProducerClient (string connectionString, string eventHubName, Azure.Messaging.EventHubs.Producer.EventHubProducerClientOptions clientOptions);
public EventHubProducerClient (string fullyQualifiedNamespace, string eventHubName, Azure.Core.TokenCredential credential, Azure.Messaging.EventHubs.Producer.EventHubProducerClientOptions clientOptions = default);

After:

public EventHubProducerClient (string connectionString, string eventHubName, Azure.Messaging.EventHubs.Producer.EventHubProducerClientOptions clientOptions);
public EventHubProducerClient (string fullyQualifiedNamespaceOrConnectionString, string eventHubName, Azure.Core.TokenCredential credential, Azure.Messaging.EventHubs.Producer.EventHubProducerClientOptions clientOptions = default);

This puts the burden of parsing the given string to determine if what was passed was a connection string with SAS details, a connection string without SAS details or an Event Hubs namespace on the client. The disadvantage is that a malformed connection string cannot be identified and will get considered as a namespace, so we will fail to throw a validation error upfront.

jsquire commented 3 years ago

Can we perhaps consider a constructor overload that takes both the connection string and a TokenCredential? The absence of the any SAS related details in the connection string can then be taken as an indicator to use the provided credential?

This will serve the need to configure SAS vs Managed Identity via a connection string without changing code and avoids the dependency on the Identity library from inside the client

Respectfully, I'm not a fan of this approach and would vote to avoid it. I think that it muddies the expectation of what developers should pass into the proposed fullyQualifiedNamespaceOrConnectionString value and how the different combinations of that parameter interact with those provided by eventHubName and authorization. For what it's worth, I feel the same about eventHubName, which I think that we can agree that we included only because of a lacking portal experience and not because we felt it was a better design to have it as a separate parameter. Of special concern for me is the potential confusion around which authorization would actually take precedence.

With respect to security, I'd advocate that the code paths need to be explicit and clear; there should be no ambiguity as to what credentials are in use allow for a potentially unintended disparity in permissions. Having this kind of split/override with parameter combinations opens up a potential security hole in that should someone inject a connection string value that includes a key/token for other publishers or with expanded permissions and have it override the intended RBAC assignments represented in the TokenCredential.

Fundamentally, I see it as the SDK's job to keep a single and clear point of authorization that requires an explicit statement of intention. The application consuming the SDK is free to make the choice that best fits its scenarios and tolerances for risk, such as extending the connection string grammar to drive the authentication/authorization flow or doing so based on other environmental parameters.

SeanFeldman commented 3 years ago

Assuming that authentication is not a part of messaging libraries and that a variant of the ConnectionStringBuilder is introduced into the library, allowing applications to parse/create connection strings agnostic of the specifics, what gaps would you see?

Yes, that would help. There's no reason library authors would need to build parsing helper classes that either already exist (looking at Storage v12) or could be provided OOTB such as in this case.

ericsampson commented 3 years ago

Was a decision reached on this topic?

ramya-rao-a commented 3 years ago

@ericsampson, Not yet. We do have a go ahead for including SharedAccessSignature in the connection string. The latest Event Hubs and Service Bus packages are support this.

We are investigating options for the rest of the problem statement here which is (stealing from @jsquire's words in another discussion)

Allow developers working with the Azure Messaging libraries to instantiate client types using a code path that adapts to changes in configuration without requiring code to be recompiled. Client creation between different environments should have minimal friction and require only simple, straightforward, and safe code that is appropriate for applications.

We will post an update here when we have something to share.

ramya-rao-a commented 2 years ago

Alright, a year later closing this thread with the approaches taken in the new SDKs across languages for Event Hubs and Service Bus

SharedAccessSignature is supported in the connection string. This will enable anyone using custom connection strings today with SharedAccessSignature to continue using it in the new SDKs
We discourage building connection strings on your own and therefore the recommended way to provide your own SharedAccessSignature will be via a constructor overload that takes AzureSasCredential. Here you create the credential object using SharedAccessSignature. Since the signature can be updated anytime later, you get the added benefit of being able to rotate your SharedAccessSignature
Additionally, we have a constructor overload that takes AzureNamedKeyCredential. Here you create the credential object using SharedAccessKey and key name. Since these values can be updated later, you get the added benefit of being able to rotate your keys

To allow a code path that adapts to changes in configuration without requiring code to be recompiled and allow client creation between different environments with minimal friction, we recommend using separate configuration entities.

For example, below is a case when customer wants to switch between environments where one uses Managed Identity or any other form of AAD auth and the other that uses SharedAccessKey or SharedAccessSignature in the connection string. This requires the customer to have 2 configurations, one for connection string and another to indicate whether or not to use AAD.

if (config.useAAD) {
    var parsed = ServiceBusConnectionStringProperties.Parse(connectionString);
    var creds = new DefaultAzureCredential();
    return new ServiceBusClient(parsed.Endpoint, creds);
}

// Supports the case when connection string is copied from the portal 
// or creating manually with an existing SharedAccessSignature
return new ServiceBusClient(connectionString);

See the below issues for Service Bus and linked PRs for details on the ServiceBusConnectionStringProperties across the 4 languages where we have new SDKs

If you have been using a custom connection string where the customization is more than SharedAccessSignature, you will need to split them into separate configuration entities as described above.

Azure / azure-sdk