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.

Sign up for GitHub

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

Jump to bottom

[identity] Use MSAL for Managed Identity #30172

Merged
merged 13 commits into from
Jul 16, 2024
Merged

[identity] Use MSAL for Managed Identity #30172

merged 13 commits into from
Jul 16, 2024

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Jun 25, 2024 •
edited
Loading

Packages impacted by this PR

@azure/identity

Issues associated with this PR

Resolves #25253

Describe the problem that is addressed by this PR

With MSAL implementing Managed Identity via the ManagedIdentityApplication, we
want to migrate our existing legacy implementation to use MSAL instead.

This PR:

  • Creates a msalMsiProvider that implements the MSAL MSI flow
  • Moves ManagedIdentityCredential to use msalMsiProvider under the hood

Some implementation details:

  • TokenExchangeMsi is not supported by MSAL and we expect to continue
    supporting it as a special handler
  • IMDS probing is not supported by MSAL and we expect to continue probing and
    failing-fast as a special handler

Provide a list of related PRs (if any)

#30045

Checklists

  • Added impacted package name to the issue description.
  • Does this PR need any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here.)
  • Added a changelog (if necessary).

Manual validation

  1. Every scenario runs against GA identity and this PR
  2. Every scenario runs using DAC and MI directly
scenario result notes
Azure VM pass
Azure Kubernetes pass only user assigned MI so far
Azure Functions pass
Azure WebApps pass
Azure Cloud Shell

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@maorleger maorleger marked this pull request as ready for review June 25, 2024 18:32
@maorleger maorleger force-pushed the msal-managed-identity-migration branch from 0386a63 to 10c5c4d Compare June 25, 2024 21:54
@KarishmaGhiya
Copy link
Contributor

Overall the PR is coming along great. I have a few questions to discuss:

  • Does MSAL have support for these - CloudShell, Azure Arc, App Configuration 2019? (i didn't see the isAvailable function for these scenarios in this file sdk/identity/identity/src/credentials/managedIdentityCredential/msalMsiProvider.ts
  • What is the default value for allowInsecureConnections? Where is that mentioned?
  • We are not giving the user the ability to switch back to legacy implementation right?

@maorleger
Copy link
Member Author

maorleger commented Jun 25, 2024 •
edited
Loading

  • Does MSAL have support for these - CloudShell, Azure Arc, App Configuration 2019? (i didn't see the isAvailable function for these scenarios in this file sdk/identity/identity/src/credentials/managedIdentityCredential/msalMsiProvider.ts
  • What is the default value for allowInsecureConnections? Where is that mentioned?
  • We are not giving the user the ability to switch back to legacy implementation right?
  • That is correct. My understanding is that we will not be following the env var approach as per our team discussions

Let me know if you have any more questions! Happy to check my understanding

@maorleger maorleger force-pushed the msal-managed-identity-migration branch from b35c4e2 to b267afd Compare June 26, 2024 16:34
xirzec
xirzec approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Nice! I like that the legacy implementation is able to be brought back until we're confident we can delete that code.

sdk/identity/identity/src/credentials/managedIdentityCredential/imdsRetryPolicy.ts Show resolved Hide resolved
sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts Show resolved Hide resolved
sdk/identity/identity/src/credentials/managedIdentityCredential/msalMsiProvider.ts Outdated Show resolved Hide resolved
sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts Outdated Show resolved Hide resolved
sdk/identity/identity/test/internal/node/managedIdentityCredential/imdsRetryPolicy.spec.ts Outdated Show resolved Hide resolved
sdk/identity/identity/test/internal/node/managedIdentityCredential/msalMsiProvider.spec.ts Outdated Show resolved Hide resolved
@maorleger maorleger mentioned this pull request Jun 26, 2024
Closed
@maorleger maorleger force-pushed the msal-managed-identity-migration branch 2 times, most recently from e60460e to a0c72fd Compare July 5, 2024 21:39
@maorleger maorleger force-pushed the msal-managed-identity-migration branch from a0c72fd to 03625ff Compare July 15, 2024 18:31
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@maorleger
Copy link
Member Author

FYI planning to merge this today regardless so please do review by EOD

@maorleger maorleger force-pushed the msal-managed-identity-migration branch from f96a6f8 to 1d4bb3c Compare July 16, 2024 17:22
@maorleger maorleger mentioned this pull request Jul 16, 2024
Open
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

1 similar comment
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@maorleger maorleger merged commit 1d9920a into Azure:main Jul 16, 2024
14 checks passed
@maorleger maorleger deleted the msal-managed-identity-migration branch July 16, 2024 22:54
This was referenced Jul 25, 2024
Closed
Merged
maorleger added a commit that referenced this pull request Aug 8, 2024
@maorleger
[core] Added calculateRetryDelay helper (#30572)
68be14a 
### Packages impacted by this PR

@azure/core-util

### Issues associated with this PR

Contributes to #30187

### Describe the problem that is addressed by this PR

Ports over the logic from @azure/core-rest-pipeline's
exponentialRetryStrategy
to a helper function that can be used by client libraries.

This code was duplicated in @azure/identity and a partner team attempted
to add exponential-backoff as a dependency to add support for this logic.

A helper function can be used to calculate the next interval, leaving
the business logic / deciding what to do next to the specific usecase

### Provide a list of related PRs _(if any)_

#30172 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christothes christothes christothes left review comments

@xirzec xirzec xirzec approved these changes

@KarishmaGhiya KarishmaGhiya KarishmaGhiya approved these changes

@ckairen ckairen Awaiting requested review from ckairen

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

@schaabs schaabs Awaiting requested review from schaabs schaabs is a code owner

@pvaneck pvaneck Awaiting requested review from pvaneck pvaneck is a code owner automatically assigned from Azure/azure-sdk-write-identity

@g2vinay g2vinay Awaiting requested review from g2vinay g2vinay is a code owner automatically assigned from Azure/azure-sdk-write-identity

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan ahsonkhan is a code owner automatically assigned from Azure/azure-sdk-write-identity

@chlowell chlowell Awaiting requested review from chlowell chlowell is a code owner automatically assigned from Azure/azure-sdk-write-identity

@mpodwysocki mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki is a code owner automatically assigned from Azure/azure-sdk-write-identity

Assignees

@maorleger maorleger

Labels
Azure.Identity
Projects
Azure Identity SDK Improvements
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use MSAL for managed identity token requests
5 participants
@maorleger @azure-sdk @KarishmaGhiya @xirzec @christothes