Skip to content

Add workload identity as a MSAL option #228

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

danielkoek
Copy link

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 Copilot AI review requested due to automatic review settings April 25, 2025 11:31
@danielkoek danielkoek requested a review from a team as a code owner April 25, 2025 11:31
@github-actions github-actions bot added ML: Core Tags changes to core libraries From Fork This PR was created from a Fork labels Apr 25, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

@microsoft-github-policy-service agree

@danielkoek danielkoek closed this Apr 25, 2025
@danielkoek danielkoek reopened this Apr 25, 2025
@danielkoek danielkoek requested a review from Copilot April 25, 2025 11:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

MattB-msft commented Apr 25, 2025

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

Does this not work for your situation?

@danielkoek
Copy link
Author

@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

@danielkoek danielkoek requested a review from Copilot April 28, 2025 09:59
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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

@danielkoek danielkoek requested a review from Copilot April 28, 2025 10:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

@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
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
From Fork This PR was created from a Fork ML: Core Tags changes to core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants