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

Update usage of MSAL in Workload Identity sample. See .NET for guidance (currently in PR). https://github.com/Azure/azure-workload-identity/tree/main/examples #636

Open
bgavrilMS opened this issue Dec 4, 2023 · 5 comments

Comments

@bgavrilMS
Copy link
Member

No description provided.

@rayluo
Copy link
Collaborator

rayluo commented Feb 22, 2024

Investigation result:

  • The existing Workload Identity sample for Python, written 3 years ago, never uses MSAL's token cache.
  • However, starting from MSAL Python 1.23+ 7 months ago, acquire_token_for_client() automatically utilizes token cache. So, that sub-optimal sample becomes acceptable now. (That being said, I'll still send out a PR there to at least bump MSAL Python's version. Currently it unnecessarily pinned to MSAL Python 1.22.)
  • A bigger uncertainty is that the current sample(s) all read a JWT from a file-on-disk. It is unclear to me how that file was generated in the workflow, and how the token was acquired from elsewhere.
    • In MSAL Python's recent Managed Identity PR, we explored an experimental approach that the CCA provides native support for workload identity.
      mi = SystemAssignedManagedIdentity()
      cca = ConfidentialClientApplication("my_client_id", client_credential=mi)
      cca.acquire_token_for_client(["scope"])
      This new pattern automatically takes care of obtaining a new federated JWT on the fly, and cache it for 1 hour. No need to fiddle with an extra file. If you are also OK with that, we can ship it in end-of-March release, and then update the workload identity sample accordingly.

@bgavrilMS
Copy link
Member Author

bgavrilMS commented Feb 22, 2024

Agreed with the bump of MSAL Py version of course.
Yes, with AKV, the token from the AKV specific identity provider is read from a file. Not sure why it was designed like this, but it's besides the point.

I do not understand your point about MI. There are 2 types of FIC:

  1. federation with an external Identity Provider. This is the case here (the provider is AKV).
  2. federation with AAD where you get a token for https://token_exchange via MSI and you use that in FIC. This is not the case here.

For 1, we have no way of understanding the lifetime of the token from the external IdP. Not sure we assume it's valid for 1h, I think in AKV case it might be, but in general case it is not. Have a look at the .NET implementation - https://github.com/Azure/azure-workload-identity/blob/main/examples/msal-net/akvdotnet/TokenCredential.cs#L33 - notice how the assertion is not read once and fed to MSAL. Instead, MSAL is fed a callback (a function pointer) which is invoked every time it needs to make the client_credentials flow.

@rayluo
Copy link
Collaborator

rayluo commented Feb 22, 2024

Thanks for clarifying the "(1) federation with external IdP (such as AKV)" and "(2) federation with AAD".

Now I realize that my experiment in my previous message was only good for "(2) federation with AAD". Even so, it seems still valuable to have that syntactic sugar, which was not that straightforward to write and test on-the-fly. It even hides the api://AzureADTokenExchange detail.

That syntactic sugar probably won't help in scenario "(1) federation with external IdP (such as AKV)". But then it won't make it any worse. In that sense, perhaps it is still OK to keep the ConfidentialClientApplication("client_id", client_credential=SystemManagedIdentity()) usage.

With regard to the file I/O, it is probably insignificant here, because the acquire_token_for_client(["scope"]) will already cache the token for up to the token lifetime. The file I/O will only occur when the token expires.

@bgavrilMS
Copy link
Member Author

Great, if file I/O happens when token expires, this is perfect. With some MSALs (.NET in particular) we had to make some changes to accommodate this - i.e. MSAL used to accept user assertion as a string only, but then we added a callback option, so that the app developer can provide a fresh user assertion whenever MSAL needs it.

@rayluo
Copy link
Collaborator

rayluo commented Feb 23, 2024

A new PR was created in that repo.

Perhaps we can close this issue here, and follow up subsequent conversation there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Commited High Priority
Development

No branches or pull requests

2 participants