Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Add MSI token revocation support for legacy sources #5139

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

Add MSI token revocation support for legacy sources #5139

wants to merge 7 commits into from

Conversation

gladjohn
Copy link
Contributor

@gladjohn gladjohn commented Feb 12, 2025 •
edited
Loading

Fixes #5138

Spec: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/docs/msiv1_token_revocation.md

This pull request includes significant changes to the Microsoft.Identity.Client library, focusing on enhancing the handling of managed identity authentication requests. The key changes involve adding support for claims and capabilities, improving token handling logic, and refactoring various classes to accommodate these new features.

Enhancements to Managed Identity Authentication:

Refactoring for Claims and Capabilities:

Updates to Managed Identity Sources:

  • Updated various managed identity source classes (AppServiceManagedIdentitySource, AzureArcManagedIdentitySource, CloudShellManagedIdentitySource, ImdsManagedIdentitySource, MachineLearningManagedIdentitySource, ServiceFabricManagedIdentitySource) to use the new CreateRequest method signature that includes AcquireTokenForManagedIdentityParameters. [1] [2] [3] [4] [5] [6] [7]

These changes collectively improve the robustness and flexibility of managed identity authentication in the Microsoft.Identity.Client library.

Testing
unit tests

Performance impact
none

Documentation

  • All relevant documentation is updated.

@gladjohn gladjohn changed the title initial Add MSI token revocation support for legacy sources Feb 12, 2025
@gladjohn gladjohn self-assigned this Feb 12, 2025
@gladjohn gladjohn mentioned this pull request Mar 10, 2025
Merged
1 task
@gladjohn gladjohn force-pushed the gladjohn/msi_v1_tr branch from 4ac538d to 579b189 Compare March 10, 2025 16:37
@gladjohn gladjohn removed the blocked label Mar 10, 2025
bgavrilMS
bgavrilMS reviewed Mar 17, 2025
View reviewed changes
src/client/Microsoft.Identity.Client/ManagedIdentity/AppServiceManagedIdentitySource.cs
{
ManagedIdentityRequest request = new(System.Net.Http.HttpMethod.Get, _endpoint);

request.Headers.Add(SecretHeaderName, _secret);
request.QueryParameters["api-version"] = AppServiceMsiApiVersion;
request.QueryParameters["resource"] = resource;

ApplyClaimsAndCapabilities(request, parameters);
Copy link
Member

Choose a reason for hiding this comment

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

Have AppService folks sing-off on a spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will remove bypass_cache and pass the bad token hash for all MSI v1 sources, based on the sign off from APP Service today

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion
Even though App Service signed off this, they are not yet actively working on this, are they? I would suggest we add this new xms_cc and token_sha256_to_refresh behaviors for only Service Fabric in this PR.

Other MIv1 providers, when they are ready, can approach us in the future, and then we will be able to add support for them by this one-liner here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of the spec sign off, they (app service) also agreed on a deliverable. We need to get this out for them to actively work on this.

bgavrilMS
bgavrilMS previously requested changes Mar 17, 2025
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Not to spec https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/docs/msiv1_token_revocation.md

@bgavrilMS
Copy link
Member

Fixes #5138

Changes proposed in this request This pull request includes several changes to the Microsoft.Identity.Client library to support claims and capabilities in managed identity requests. The most important changes involve adding a new Claims property, modifying request creation methods to include this property, and implementing a new method to apply claims and capabilities to requests.

Support for Claims and Capabilities:

Request Creation and Handling:

  • src/client/Microsoft.Identity.Client/ManagedIdentity/AbstractManagedIdentity.cs: Modified the CreateRequest method to accept AcquireTokenForManagedIdentityParameters and added the ApplyClaimsAndCapabilities method to set request parameters based on claims and capabilities. [1] [2] [3]
  • Updated various managed identity source classes (AppServiceManagedIdentitySource, AzureArcManagedIdentitySource, CloudShellManagedIdentitySource, ImdsManagedIdentitySource, MachineLearningManagedIdentitySource, ServiceFabricManagedIdentitySource) to use the new CreateRequest method signature and apply claims and capabilities. [1] [2] [3] [4] [5] [6]

Testing Enhancements:

Testing unit tests

Performance impact none

Documentation

  • All relevant documentation is updated.

@gladjohn gladjohn marked this pull request as ready for review March 19, 2025 18:29
@gladjohn gladjohn requested a review from a team as a code owner March 19, 2025 18:29
@gladjohn gladjohn requested a review from bgavrilMS March 19, 2025 18:29
@gladjohn gladjohn dismissed bgavrilMS’s stale review March 20, 2025 17:31

re-review requested

gladjohn
gladjohn commented Mar 21, 2025
View reviewed changes
src/client/Microsoft.Identity.Client/ManagedIdentity/AppServiceManagedIdentitySource.cs
@@ -13,7 +15,7 @@ namespace Microsoft.Identity.Client.ManagedIdentity
internal class AppServiceManagedIdentitySource : AbstractManagedIdentity
{
// MSI Constants. Docs for MSI are available here https://docs.microsoft.com/azure/app-service/overview-managed-identity
private const string AppServiceMsiApiVersion = "2019-08-01";
private const string AppServiceMsiApiVersion = "2025-03-30";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to new api-version, we cannot merge this PR until App Service api-version has been updated globally. Marked the PR as do not merge. Also do not close this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ ⚠ 🚧 ❗ ⛔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will fail the integration test as there is no new API-VERSION in APP Service, yet. This failure is expected.

neha-bhargava
neha-bhargava reviewed Apr 1, 2025
View reviewed changes
src/client/Microsoft.Identity.Client/ManagedIdentity/AppServiceManagedIdentitySource.cs
@@ -13,7 +15,7 @@ namespace Microsoft.Identity.Client.ManagedIdentity
internal class AppServiceManagedIdentitySource : AbstractManagedIdentity
{
// MSI Constants. Docs for MSI are available here https://docs.microsoft.com/azure/app-service/overview-managed-identity
private const string AppServiceMsiApiVersion = "2019-08-01";
private const string AppServiceMsiApiVersion = "2025-03-30";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this version of app service endpoint the equivalent to MITS endpoint mentioned in the spec?

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@rayluo rayluo rayluo left review comments

@neha-bhargava neha-bhargava neha-bhargava left review comments

@bgavrilMS bgavrilMS Awaiting requested review from bgavrilMS

At least 2 approving reviews are required to merge this pull request.

Assignees

@gladjohn gladjohn

Labels
do not merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add MSI v1 token revocation support
5 participants
@gladjohn @bgavrilMS @rayluo @neha-bhargava @GladwinJohnson