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

Client secret and certificate credentials use MSAL ConfidentialClientApplication #13215

Merged
merged 6 commits into from
Aug 25, 2020

Conversation

chlowell
Copy link
Member

This implements the synchronous ClientSecretCredential and CertificateCredential atop msal.ConfidentialClientApplication to enable them to leverage MSAL features. CertificateCredential temporarily relies on an MSAL implementation detail to support password protected certificates. MSAL will formally support passwords before azure-identity 1.5.0 (tracked at AzureAD/microsoft-authentication-library-for-python#232), so that's a temporary workaround.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Aug 19, 2020
@chlowell chlowell requested a review from schaabs as a code owner August 19, 2020 16:01
@@ -73,7 +73,7 @@
install_requires=[
"azure-core<2.0.0,>=1.0.0",
"cryptography>=2.1.4",
"msal<2.0.0,>=1.3.0",
"msal<1.5.0,>=1.3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The latest msal is 1.4.3. The purpose of this is to ensure the workaround for msal not accepting encrypted private keys isn't broken by the next minor version, which will accept encrypted keys. That's expected to land before we want to mark 1.5.0 stable, so I'll revert this change before then.

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not quite clear to me why we only update the sync version but no async as well?

@chlowell
Copy link
Member Author

msal has no async API. When it does, we'll make similar changes to the async credentials.

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what's the benefit to use msal (sync only)?

@chlowell
Copy link
Member Author

This allows the credentials to expose MSAL features to the Azure CLI without reimplementing them.

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we want to have some "sync-only" features?

@chlowell
Copy link
Member Author

For the time being. It would be better to have the same API for async applications--and eventually we will--but reimplementing parts of MSAL is unsustainable (see SharedTokenCacheCredential). And sync-only features aren't unprecedented, for example there are no async user credentials.

@chlowell chlowell requested a review from mccoyp August 25, 2020 17:57
return token

def _get_auth_client(self, tenant_id, client_id, **kwargs):
return AadClient(tenant_id, client_id, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We disabled proactive-refreshing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base class inherits GetTokenMixin, which handles refreshing.

@chlowell chlowell merged commit a1933fc into Azure:master Aug 25, 2020
@chlowell chlowell deleted the msal-client-credentials branch August 25, 2020 20:43
@joshfree joshfree mentioned this pull request Sep 4, 2020
4 tasks
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Mar 2, 2021
[Hub Generated] Review request for Microsoft.DeviceUpdate to add version preview/2020-03-01-preview (Azure#13215)

* New Readme Config File

* New Go Language Readme Config File

* New Azure AZ Readme Config File

* New Azure CLI Readme Config File

* New Typescript Language Readme Config File

* New Python Language Readme Config File

* New C# Language Readme Config File

* New AzureResourceSchema Readme Config File

* New Swagger Spec File

* New Swagger Example Spec File

* Release of Device Update for IoT Hub control plane specification.

* Fixing prettier issues.

* Updating control plane swagger

* Update deviceupdate.json

* Update deviceupdate.json

* Update deviceupdate.json

* Update deviceupdate.json

* add the multiapi section

* Update Accounts_Delete.json

* Update Instances_Delete.json

Co-authored-by: David Pokluda <[email protected]>
Co-authored-by: Alexander Batishchev <[email protected]>
Co-authored-by: ArcturusZhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants