search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.36k stars 4.71k forks source link

[BUG] MsiAccessTokenProvider & AzureServiceTokenProvider.GetAuthenticationResultAsync not honoring tenantId parameter #9498

Closed rggammon closed 3 years ago

rggammon commented 4 years ago

Describe the bug I am trying to use MSI with Bot Framework, replacing their AdalAuthenticator.

I am calling:

var authResult = await _tokenProvider.GetAuthenticationResultAsync(AuthenticationConstants.ToChannelFromBotOAuthScope, "d6d49420-f39b-4df7-a1dc-d59a935871db");

… where "d6d49420-f39b-4df7-a1dc-d59a935871db" is a hard-coded tenant in the bot framework SDK, but the token I get back via MsiAccessTokenProvider is …

"iss": "https://sts.windows.net/72f988bf-86f1-41af-91ab-2d7cd011db47/" (Microsoft tenant)

Exception or Stack Trace N/A

To Reproduce Call GetAuthenticationResultAsync provding a tenantId

Code Snippet See above

Expected behavior Issuer should contain d6d49420-f39b-4df7-a1dc-d59a935871db as the tenant, as provided to the call to GetAuthenticationResultAsync

Screenshots N/A

Setup (please complete the following information):

Additional context AdalAuthenticator (which I am replacing) is -

https://github.com/microsoft/botbuilder-dotnet/blob/master/libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

rggammon commented 4 years ago

If it matters - d6d49420-f39b-4df7-a1dc-d59a935871db is DefaultChannelAuthTenant (botframework.com)

https://github.com/microsoft/botbuilder-dotnet/blob/633ddc786b1b33b8618f2326ac077861672f850e/libraries/Microsoft.Bot.Connector/Authentication/AuthenticationConstants.cs#L28

rggammon commented 4 years ago

https://docs.microsoft.com/en-us/azure/bot-service/rest-api/bot-framework-rest-connector-authentication?view=azure-bot-service-4.0#step-1-request-an-access-token-from-the-azure-ad-v2-account-login-service

nonik0 commented 4 years ago

Hi @rggammon, based on the last link you provided, it looks like you need to request a token with the App ID and App Password after you register your bot.

AppAuth will always default to requesting a token for the managed identity when deployed to an App Service or VM, and this identity will always live in your subscription's tenant. Unfortunately, due to the abstracting nature of AzureServiceTokenProvider, the tenantId parameter is not always honored, such as in this scenario.

However, in order to configure the AzureServiceTokenProvider to request a token for your bot's app, you can use the following AppAuth connection string, where you would your bot's App ID and Password you should have after following the steps from the earlier link: RunAs=App;AppId={AppId};TenantId={TenantId};AppKey={ClientSecret}

Let me know if this doesn't help or you have any other questions.

rggammon commented 4 years ago

Thanks Nick. My larger goal is to deploy a Web App Bot from an ARM template, and being able to use a managed identity in this scenario would be beneficial, because then the bot owner won't need to pre-create the app registration, nor worry about secret rotation.

Options I could see here -

  1. MSI could allow the caller to pass an authority or tenant id to use, and the AzureServiceTokenProvider could then pass this in the REST call
  2. Bot Framework could change their services to accept tokens from tenants other than botframework.com
  3. As a follow-up step, a 2nd app registration could be create, and client_id/client_secret could be manually configured into key vault. The 2nd app registration could then be used with AdalAuthenticator to get the token from the botframework.com tenant.

I could open an issue in botbuilder-dotnet repo with the 2nd suggestion, but it would be helpful to know if this (specifying the tenant/authority, in addition to client_id) is a valid "feature request" for MSI, especially given that the sdk accepts a tenantId parameter?

Or maybe option 3 (2 appId's) are the way to go here - unfortunately, the 2nd appId would be "unmanaged" & the setup process would be manual, but at least the credentials would be in keyvault.

rggammon commented 4 years ago

I will go with option 3, I think I will need an additonal appId anyway to handle user delegated permissions, auth code flows.

https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/known-issues covers questions on integration with ADAL/MSAL (not yet), and https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/services-support-managed-identities does not include bot services.

For the original bug, perhaps consider throwing an exeption of some kind, if the tenant id parameter is provided but not honored, rather than returning a token for the wrong tenant? Otherwise, this can be closed. Thanks!

varunsh-coder commented 4 years ago

The right thing to do is to make a change to throw an exception if the tenant id is specified, but not used. This will be a breaking change, so we will evaluate the timeline and get back on this thread.

anaismiller commented 3 years ago

This will not be addressed. If this is a blocking issue, please use the new Azure.Identity library. More information available here: AppAuthentication to Azure.Identity Migration Guidance