Add workload identity as a MSAL option

[danielkoek]

Hi,

I can see that there is no option for a workload identity for agents, we are running on AKS so we would like to add a workload identity instead, this will make it more secure then federation/clientsecrets

[Copilot]

Pull Request Overview

This PR introduces support for workload identity authentication for agents running on AKS. The key changes include:

• Adding a new branch in MsalAuth.cs to handle WorkloadIdentity without performing client assertion.
• Enhancing the ConnectionSettings validation to check required fields for WorkloadIdentity.
• Adding WorkloadIdentity as a new enum value in AuthTypes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/Authentication/Authentication.Msal/MsalAuth.cs	Added a new branch for WorkloadIdentity token handling
src/libraries/Authentication/Authentication.Msal/Model/ConnectionSettings.cs	Updated configuration validation to support WorkloadIdentity
src/libraries/Authentication/Authentication.Msal/Model/AuthTypes.cs	Added WorkloadIdentity to the AuthTypes enum

[danielkoek]

@microsoft-github-policy-service agree

[Copilot]

Pull Request Overview

This PR introduces a WorkloadIdentity option for MSAL authentication to enhance security on AKS by leveraging workload identity instead of federation/client secrets.

• Added a new branch in MsalAuth.cs to handle the WorkloadIdentity authentication.
• Extended ConnectionSettings.cs to enforce required fields for WorkloadIdentity.
• Updated the AuthTypes enum to include WorkloadIdentity.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/libraries/Authentication/Authentication.Msal/MsalAuth.cs	Added a branch for AuthTypes.WorkloadIdentity with a comment clarifying that no additional action is required.
src/libraries/Authentication/Authentication.Msal/Model/ConnectionSettings.cs	Added configuration validation for WorkloadIdentity requiring ClientId and either Authority or TenantId.
src/libraries/Authentication/Authentication.Msal/Model/AuthTypes.cs	Extended enum by adding WorkloadIdentity.

[MattB-msft]

@danielkoek
Thanks for your suggestion here.
However, we already support this capability via Federated Identity Credentials.

Does this not work for your situation?

[danielkoek]

@danielkoek Thanks for your suggestion here. However, we already support this capability via Federated Identity Credentials.

Does this not work for your situation?

Federation and workloads identity are very different, correct me if I am wrong, but I defo don't need a "FederationId", But i also don't need to supply a password OR a cert, you can use an assertion, so i added that now:
As an example:
https://github.com/Azure/azure-workload-identity/blob/main/examples/msal-net/akvdotnet/TokenCredential.cs
Docs about it:
https://learn.microsoft.com/en-us/azure/aks/workload-identity-overview?tabs=dotnet

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull Request Overview

This PR adds a workload identity authentication option to MSAL to support agents running on AKS for improved security.

• Implements a new branch in the MSAL authentication flow to read and cache the workload identity token.
• Adds a FederatedTokenFile property and validation in ConnectionSettings.
• Extends the AuthTypes enum to include WorkloadIdentity.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/Authentication/Authentication.Msal/MsalAuth.cs	Implements workload identity token retrieval with caching logic.
src/libraries/Authentication/Authentication.Msal/Model/ConnectionSettings.cs	Adds FederatedTokenFile property and updates configuration validation for workload identity.
src/libraries/Authentication/Authentication.Msal/Model/AuthTypes.cs	Adds the WorkloadIdentity option to the authentication types.

Comments suppressed due to low confidence (1)

src/libraries/Authentication/Authentication.Msal/MsalAuth.cs:40

• [nitpick] The variable '_lastJwtWorkLoadIdentity' mixes casing ('WorkLoad') that may be inconsistent with the enum 'WorkloadIdentity'. Consider renaming it to '_lastJwtWorkloadIdentity' for clarity and consistency.

private string _lastJwtWorkLoadIdentity = null;

[danielkoek]

@MattB-msft
I have checked it on our system now too and we definitely cant use the federated approach, so for now i have used a local assembly that "mocks" this and that works!

[tracyboehrer]

@danielkoek We still see this, Daniël. Considering it, but it may not be until after Build (May 19) that we have space to get to it.

[MattB-msft]

@danielkoek
Yes.. we want to dig more into this before accepting it, As @tracyboehrer said, It will be post build before we get into it..
We will take a look through this more in depth then.

pre that,
can you provide more information about your configuration? What are you running your container in / hosting so we can set this up and understand the configuration to make sure we account for it in the future.

[danielkoek]

@danielkoek Yes.. we want to dig more into this before accepting it, As @tracyboehrer said, It will be post build before we get into it.. We will take a look through this more in depth then.

pre that, can you provide more information about your configuration? What are you running your container in / hosting so we can set this up and understand the configuration to make sure we account for it in the future.

Yea no problem, i think i already mentioned it, workload identity: https://azure.github.io/azure-workload-identity/docs/installation/azwi.html

Just normal AKS cluster with a SA setup to have the workload identity loaded in (azwi will do the SA part too), the example used the token file, so that is what i did in my own implementation as well. Our repo for this is actually public: https://github.com/Uniphar/devops-teams-notifications-api, you can see how i "fixed" it there too.

[tracyboehrer]

@danielkoek Thanks for this contribution and inspiration. We are going to add this but implemented this using AzureIdentityForKubernetesClientAssertion and some additional unit testing.

See: #320

[danielkoek]

@danielkoek Thanks for this contribution and inspiration. We are going to add this but implemented this using AzureIdentityForKubernetesClientAssertion and some additional unit testing.

See: #320

If that is the way you want to do by all means, thought it would be nice to do some contribution, seems that you just took this code and put a few unit tests too it... if you need me to test anything let me know we have it working now on multiple environments

[tracyboehrer]

@danielkoek Has this version been tested in a live Agent?

[danielkoek]

@danielkoek Has this version been tested in a live Agent?

@tracyboehrer We're currently using this specific implementation: https://github.com/Uniphar/devops-teams-notifications-api/blob/main/src/Teams.Notifications.Api/MsalAuthChanged.cs. It's almost a 1:1 copy, with the main differences being how we handle configuration and some minor adjustments to streamline it for our specific use case.

I don't believe there are PR-specific NuGet builds available, so I'm not entirely sure of the best way to test this change. If there's any documentation or guidance on how to test changes from a PR (e.g., using a local build or NuGet feed), I'd be happy to take a look.

[MattB-msft]

@danielkoek we discussed this more internally, We are going to accept this PR so that you do get credit for it :) and make any changes we need to based on this change.

thanks again for your contribution and your patience as we worked out some kinks in our processes.