Move Service Bus Pubsub/Binding to common auth

[halspang]

Description

Both the pubsub and input/output binding for Azure Service Bus were
connecting via a connection string. This is still supported but will
now fallback to using AAD from the common auth library. This is also
the recommended auth pattern going forward.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Partial: #1103

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: Update Component Spec for Azure Service Bus Pubsub and Bindings docs#1867

[berndverst]

FYI, @halspang and I successfully verified this works with managed identity now

apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
  name: azure-servicebus
spec:
  type: pubsub.azure.servicebus
  version: v1
  metadata:
  - name: namespaceName
    value: "dapr2-conf-test-servicebus"
  - name: consumerID
    value: "helloworld"

We ran the conformance test in an environment with managed identity where the identity was given "Azure Service Bus Data Owner" role for the particular Service Bus Namespace

[ItalyPaleAle]

Thank you for doing this!

[CodeMonkeyLeet]

Per the PR template, please make sure there's a docs issue for this, specifically the interaction with the new metadata property namespaceName.

For my understanding, do we think it'll be a common scenario to have the same config deployed in different environments where the fallthrough behavior is desirable, or is this how other Azure components handle selection of multiple auth options? If not, I generally advise that they be mutually exclusive properties that can be more clearly enforced at input time.

[berndverst]

@CodeMonkeyLeet the commonAuth used across all of Azure SDKs (and hence also implemented in our common auth logic is):

1. Try Service Principal
2. Try Certificate
3. Try Managed Identity

However the service specific connection methods take precedence before this AAD options.

Note that this new variable is necessary for AAD to request the correctly scoped Auth token. There is no way around it unfortunately.

AAD should generally be the preferred method to connect to services where available.

[CodeMonkeyLeet]

@berndverst Thanks for the info, but I'm not sure if that answers my question, which is specifically about the design of the configuration options exposed by Dapr to the end user in the Service Bus components. It might be easier to correct my understanding:

• These are the first components where a component-specific metadata property (namespaceName) must be provided to opt into AAD-based auth.
  • While there are common metadata properties defined for the authentication/azure library, they do not need to be specified for the commonAuth logic you describe to be exercised (i.e. because Managed Identity could be present, which is not parameterized by the component config).
  • Since this opt-in may be implicit in other cases, all existing Dapr Azure components using commonAuth prioritize service-specific connection methods over commonAuth, as indicated by the comment:

    However the service specific connection methods take precedence before this AAD options.

• Given that namespaceName is both component-specific and gates the use of AAD-based auth, these components have the option to require express their intent to use AAD-based auth or not more explicitly.

  • Specifically, if users have the expectation that:

    AAD should generally be the preferred method to connect to services where available.

    Then the existing behavior of using the connection string when both are specified is counterintuitive. It can be modified to require the developer to choose one or the other, so the question is, should these components be requiring that?

[halspang]

@CodeMonkeyLeet - Here's the docs PR: dapr/docs#1867

As for the counterintuitive nature between the connection string and AAD, the reason I went for that was to make the user explicitly opt into the feature. In order to do so, they have to not only add the new field but remove the old one. To me, this both honors the existing method of auth and makes the opt-in more explicit as it requires the full removal of the old method.

I do understand that this can be counterintuitive though, so I'm actually all for introducing a standard here for actually opting into the feature instead explicitly instead of implicitly. In this case, I'd prefer to have a label in the yaml that states the desired auth mode (component, AAD, etc). The only caveat with this is that if any component went out in 1.4 with the auth library, it'd be weird to add the explicit field. I'm pretty sure at least one went out doing this (azure keyvault?).

What do you think? I'm also curious to hear your opinions @berndverst and @ItalyPaleAle.

[ItalyPaleAle]

@CodeMonkeyLeet @halspang I wrote original common auth layer.

To my understanding, the "original" method of authentication for ServiceBus was providing a connection string. This is a "URL-encoded-ish" string that contains both a shared key and other metadata, including the name of the namespace.

When using AAD, the auth system (the AAD service) provides the credentials that replace the shared key. However they're not able to provide the namespace, so the developer needs to pass it themselves. I am not an expert on SB but I don't believe there's an alternative to this, can you confirm that?

In this case, IMHO it's less about making the developer need to opt-into AAD auth, but rather requiring a way to provide the missing information. I think it's ok if namespaceName and connectionString are NOT mutually exclusive, as it should be possible to pass a namespaceName even if there's a connection string: in that case, it will simply be ignored (and I agree this is something we should document).

As for making AAD auth opt-in in all cases, for all components, my vote on this is on no. Backwards compatibility aside, the idea is that with AAD-based auth your app will "just work" without you needing to do anything in particular, as the auth comes from the environment. This could be an Ops person manually injecting the AAD credentials (the clientId/clientSecret or the certificate) in the environment (enforcing the idea of separation of roles), or even more transparently the app could use MSI if it runs on a service that supports that. In this sense, the choice of not supplying a connection string or shared key on its own is an implied choice of relying on AAD.

[halspang]

@CodeMonkeyLeet - So is there anything else you want to see here? I'm fine making the options mutually exclusive to make the preference more explicit but can also just document everything.

[CodeMonkeyLeet]

@halspang Until someone overrules me, I'd say make them mutually exclusive, on the basis that it's easier to relax restrictions later as a non-breaking change. ☺

@ItalyPaleAle Total side conversation here, but does az ad sp create-for-rbac --skip-assignment help simplify your instructions? (Don't need a separate credential reset step to use client secret) It creates a SP with no default Contributor role assignment to the subscription, and is stated as the intended default behavior in the future (presumably on the 3.0 major version update).

[ItalyPaleAle]

@CodeMonkeyLeet I did not know about --skip-assignment. Looks like it should work! I've opened an issue here: dapr/docs#1872 feel free to assign it to me

[berndverst]

@ItalyPaleAle --skip-assignment definitely will work :)

[berndverst]

@CodeMonkeyLeet @halspang so what does making these options mutually exclusive mean? Throw an error if both are used? I suppose that would be fine.

I see it as mostly a documentation concern. Something like:

• shared access tokens are already scoped to a Service Bus namespace and it serves no purpose to provide the namespace
• if AAD authentication is desired, pick one of the methods described here (https://docs.dapr.io/developing-applications/integrations/cloud-providers/authenticating-azure/) and additionally provide the namespace

[daixiang0]

Could use constant for "serviceBus"? I do not find where to define this setting either.

[CodeMonkeyLeet]

LGTM, thanks for the fixes!

[ItalyPaleAle]

FYI there's a new "track 2" SDK for Azure Service Bus: https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/messaging/azservicebus

Eventually the component will need to be updated to that (and pending merge of #1290 for auth support)

[codecov]

Codecov Report

Merging #1201 (39bcc7e) into master (09fb60c) will increase coverage by 0.20%.
The diff coverage is 21.90%.

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
+ Coverage   35.02%   35.22%   +0.20%     
==========================================
  Files         148      148              
  Lines       12825    12709     -116     
==========================================
- Hits         4492     4477      -15     
+ Misses       7853     7749     -104     
- Partials      480      483       +3     

Impacted Files	Coverage Δ
authentication/azure/auth_amqp.go	0.00% <0.00%> (ø)
bindings/aws/kinesis/kinesis.go	2.61% <0.00%> (ø)
bindings/azure/eventgrid/eventgrid.go	3.84% <0.00%> (ø)
bindings/mqtt/mqtt.go	25.00% <0.00%> (-1.42%)	⬇️
authentication/azure/auth.go	40.35% <1.96%> (-15.75%)	⬇️
...indings/azure/servicebusqueues/servicebusqueues.go	11.02% <9.09%> (+0.01%)	⬆️
pubsub/azure/servicebus/servicebus.go	29.75% <20.00%> (-0.54%)	⬇️
secretstores/azure/keyvault/keyvault.go	36.25% <33.33%> (+2.00%)	⬆️
pubsub/rabbitmq/rabbitmq.go	57.14% <46.03%> (-7.15%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1be130...39bcc7e. Read the comment docs.