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

Add IAuthenticator to allow for plugging in of Managed Service Identity #3255

Merged
merged 2 commits into from
Feb 24, 2020

Conversation

rggammon
Copy link
Member

Azure App Service (and several other resource types) has a Managed Service Identity feature which simplifies management of application identity - see Microsoft.Azure.Services.AppAuthentication for more information.

By having AdalAuthenticator derive from an interface, I can then inject my own MsiAuthenticator that uses AzureServiceTokenProvider to get the token to call the channels.

I can inject my MsiAuthenticator via overriding BuildCredentialsAsync on BotFrameworkHttpAdapter and returning a MsiAppCredentials class that, in turn, returns the MsiAuthenticator

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2020

Pull Request Test Coverage Report for Build 102997

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.6%) to 79.557%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder.Dialogs/ObjectPath.cs 1 86.46%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/OAuthPrompt.cs 1 56.4%
/libraries/Microsoft.Bot.Builder/Inspection/InspectionMiddleware.cs 1 78.89%
/libraries/Microsoft.Bot.Expressions/BuiltInFunctions.cs 1 93.12%
/libraries/Microsoft.Bot.Expressions/LRUCache.cs 1 82.93%
/libraries/Microsoft.Bot.Expressions/TriggerTrees/Trigger.cs 1 85.09%
/libraries/Microsoft.Bot.Builder.Azure/CosmosDbStorageOptions.cs 2 75.0%
/libraries/Microsoft.Bot.Builder/MemoryTranscriptStore.cs 2 86.09%
Totals Coverage Status
Change from base Build 102747: -0.6%
Covered Lines: 10850
Relevant Lines: 13638

💛 - Coveralls

@johnataylor
Copy link
Member

@rggammon any update on this? The build is broken. Do you still want to move forward with this?

…ty code (Microsoft.Azure.Services.AppAuthentication)
@rggammon
Copy link
Member Author

@johnataylor - thank you for the ping :). I had some API compatibility issues, which I have resolved.

@cleemullins
Copy link
Contributor

@carlosscastro, as the owner of all things ADAL, can you take a look at this? Seems reasonable set of changes to me, but your perspective (and corresponding issues to open in JS and Python) are appreciated.

@cleemullins
Copy link
Contributor

@carlosscastro, this seems reasonable. CAn you take a look?

@carlosscastro
Copy link
Member

These are great. Let's get them in, thanks for your contribution!!!

Could you share your MsiAuth implementation of the IAuthenticator interface and a some bot where you use it if you have any? I'm looking into MSI and potential ways of exposing support, so the timing is great.

@carlosscastro
Copy link
Member

@rggammon quick reminder: Could you share your MsiAuth implementation of the IAuthenticator interface and a some bot where you use it if you have any? I'm looking into MSI and potential ways of exposing support, so the timing is great.

@rggammon
Copy link
Member Author

@carlosscastro - https://github.com/rggammon/botframework-solutions/blob/msi/skills/csharp/experimental/restaurantbookingskill/Authentication/MsiAuthenticator.cs is an example.

This should work locally. An issue you'll hit when deployed to an appservice is Azure/azure-sdk-for-net#9498 where the tenantId parameter doesn't work, and to talk to ABS, you need to use the botframework.com tenant.

But, this is still useful if the bot is being called as a skill, and allows you to deploy via a "Deploy to Azure" type button as seen on https://github.com/Azure/azure-quickstart-templates samples.

And, I can imagine how, if ABS had a client_id, a bot could run an S2S auth flow to get a token in the bot's tenant instead of the botframework.com tenant, which could get this working for ABS as well.

carlosscastro pushed a commit that referenced this pull request Mar 3, 2020
…ty (#3255)

* Add IAuthenticator to allow for plugging in of Managed Service Identity code (Microsoft.Azure.Services.AppAuthentication)

* Fix API compatibility issues
carlosscastro pushed a commit that referenced this pull request Mar 4, 2020
…ty (#3255)

* Add IAuthenticator to allow for plugging in of Managed Service Identity code (Microsoft.Azure.Services.AppAuthentication)

* Fix API compatibility issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants