Azure-sdk-for-js: Service Bus library not working with MSITokenCredentails

[mitchdenny]

• Package Name: @azure/service-bus
• Package Version: 1.0.2
• Operating system: Linux (Ubuntu 18.04 LTS)
• [x ] nodejs:
  
  
  
  • version: 12.4.0
  
  
• [ ] browser
  
  
  
  • name/version:
  
  
• [ ] typescript
  
  
  
  • version:
  
  
• Is the bug related to documentation in
  
  
  
  • [ ] README.md
  
  
  • [ ] source code documentation
  
  
  • [ ] SDK API docs on https://docs.microsoft.com
  
  

Describe the bug
The @azure/service-bus library does not appear to work with MSITokenCredentials. If I use @azure/ms-rest-nodeauth to authenticate using loginWithVmMSI and set the resource: option to https://servicebus.azure.net, the code will blow up when I go to receive messages.

The following repo contains a comprehensive repro:

https://github.com/mitchdenny/pipeline-historian/blob/022867b1ca822bbd905e3d6a33583df5b9fa194a/package.json

Additional context
We've already discussed this within the team. The root cause here is some logic in the amqp-common module - specifically this line:

https://github.com/Azure/amqp-common-js/blob/b82a5e5b5492cadf4988d538f7e67d66148f9503/lib/auth/aad.ts#L42

Basically it is stomping on the resource and setting it to eventhubs which is why it blows up.

[ramya-rao-a]

Thanks @mitchdenny

The suggested fix here is to update the if check at https://github.com/Azure/amqp-common-js/blob/b82a5e5b5492cadf4988d538f7e67d66148f9503/lib/auth/aad.ts#L42 to throw an error if resource is not set on the credential object. This is because we expect the user to pass in the right audience rather than try to set a default on our end.

Pending discussion is whether we will be re-opening the old https://github.com/Azure/amqp-common-js repo to do this fix or go to an older commit in azure-sdk-for-js to a time before amqp-common was renamed to core-amqp

[ramya0820]

The suggested fix here is to update the if check at https://github.com/Azure/amqp-common-js/blob/b82a5e5b5492cadf4988d538f7e67d66148f9503/lib/auth/aad.ts#L42 to throw an error if resource is not set on the credential object. This is because we expect the user to pass in the right audience rather than try to set a default on our end.

@mitchdenny @ramya-rao-a @amarzavery
Regarding discussion at https://github.com/Azure/azure-sdk-for-js/pull/4098#pullrequestreview-255555350, the initially suggested fix seems sufficient to me as there may be other usecases/consumers for amqp-common in future that may miss setting the resource?

Also, this throwing of error doesn't seem be a breaking change as we are passing in the 'tokenAudience' from the SDKs in both tests and samples. Is there any other use-case that's being missed out? If it's about EventHubs users out in public that we may not be aware of, should we still use the EventHubs default value and mention that in error message so that Service Bus users know what to do?

[ramya-rao-a]

If it's about EventHubs users out in public that we may not be aware of, should we still use the EventHubs default value and mention that in error message so that Service Bus users know what to do?

Yes, my comment on breaking change is for Event Hub users in the public who are using MSI credentials but without passing the audience.

Yes, we should use the Event Hubs default value, but in Event Hubs library before calling the AadTokenProvider and not in amqp-common.

The error message is excessive if both Event Hubs and Service Bus pass the right value for audience/scope

Users of any kind of AAD credentials should ideally be passing the right audience. If they don't, then the service will return appropriate errors. Therefore, we neednt throw an explicit error.

[ramya0820]

Closed with #4146