Adding FMI source to MI app #5299

trwalke · 2025-05-21T05:00:52Z

This PR is adding WithServiceFabricFmi() which will be used to enable MI applications to detect the FMI service fabric endpoints.
This API is only intended to be used by MISE for the FMI credential.
Fixes # https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3245126/

Changes proposed in this request
Please see proposal here: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/5309/files

Testing

Performance impact

Documentation

All relevant documentation is updated.

trwalke · 2025-05-21T05:03:07Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs

            string identityEndpoint = EnvironmentVariables.IdentityEndpoint;

            requestContext.Logger.Info(() => "[Managed Identity] Service fabric managed identity is available.");

-            if (!Uri.TryCreate(identityEndpoint, UriKind.Absolute, out Uri endpointUri))
+            if (requestContext.ServiceBundle.Config.IsFmiServiceFabric)


I was considering letting MSAL autodetect the correct env variable, but I am not sure if it is a good idea. Both env variables may be set (IDENTITY_ENDPOINT and APP_IDENTITY_ENDPOINT), so I don't want to introduce bugs. Adding an api for this that will only be used from MISE will make this more robust.

Let's take a step back and think how we'd go about doing this in MSAL if we were to productize getting the FMI credential.

The FMI credential is just a token with a specific audience. And it can use different endpoints.

Can we use the existing MSI APIs, and have different logic based on the audience? i.e. if app developer requests token for api://AzureFMITokenExchange (or the GUID format) - then add your custom logic?

Also, let's add the "experimental features" to this?

@bgavrilMS I understand what you are saying, but since this scenario isnt intended to use a resource and is simply acquiring the token from MITS, all of the additional application logic for resources/scopes/caching is not needed. I would also would be concerned if someone wanted to use MI to exchange an FMI credential for an access token using api://AzureFMITokenExchange, but instead they just get another FMI Credential. so, maybe instead of using the fmi token exchange resource, we can create some string like "FmiMitsAcquisition" or something so that no one will trigger this logic in MSAL by accident? But if you feel like no one will ever use api://AzureFMITokenExchange with a MI app, this this will work.

I'm ok with an experimental API mi.AcquireFmiCredential(). Not sure I see a scenario where api://AzureFMITokenExchange can be mis-used either.

@rayluo - thoughts on this?

@trwalke - can you put toghter a small design doc for this and let's discuss with the team?

@bgavrilMS Added here
#5309

This reverts commit 44e34c5.

This reverts commit d1c9a99.

Adding experimental feature requirement.

...client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForManagedIdentityParameterBuilder.cs

...t/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForManagedIdentityParameters.cs

...ent/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricFederatedManagedIdentitySource.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs

Clean up

MZOLN · 2025-06-24T18:25:18Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs

+                request.QueryParameters["api-version"] = ServiceFabricMsiApiVersion;
+                request.QueryParameters["resource"] = resource;
+
+                switch (_requestContext.ServiceBundle.Config.ManagedIdentityId.IdType)


Shouldnt there be a default case? We could log the unexpected, that could give us additional information when troubleshooting.

Technically yes, but this ID is a required parameter for the managed identity application. So it will always be set to something unless there is a catastrophic failure.

It's still a good practice to throw an exception @trwalke on default. It will protect against a case where a new enum parameter is added.

bgavrilMS · 2025-07-01T10:13:02Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs

-        internal static Lazy<HttpClient> _httpClientLazy;
-
-        public static AbstractManagedIdentity Create(RequestContext requestContext)
+        public static AbstractManagedIdentity Create(RequestContext requestContext, bool isFmiServiceFabric = false)


I don't think this is right. The source detection should not interfere with isFmi flag. The MSI source should be expected to be cached (statically, in memory) by MSAL.

In other words, each MSI request can either go to the normal endpoint or to the fmi endpoint.

I dont think this is the case. The source detection is effectively turned off if IsFmi is enabled and only the FMI source will be used.
See ManagedIdentityClient

This parameter specifically is to change this source to only do FMI.

Also, when you say cached (statically, in memory), can more that one of these source objects be cached? meaning, can there be one for regular SF and one for FMI?

src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs

bgavrilMS · 2025-07-01T10:18:06Z

tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ServiceFabricTests.cs

+        }
+
+        [TestMethod]
+        public async Task ValidateThatFmiCredentialCanBeAcquiredFromMits()


You need a test where you ask for a FMI credential, then for an FMI token (and repeat).

I didnt add this because this code is only responsible for acquiring the credential. The full E2E is already tested in MISE. Are we sure we also want to add the full e2e test here?

bgavrilMS

source detection should not depend on isFmi

bgavrilMS · 2025-07-09T17:58:50Z

...client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForManagedIdentityParameterBuilder.cs

+
+            if (resource.Equals("api://AzureFMITokenExchange/.default", StringComparison.OrdinalIgnoreCase))
+            {
+                Parameters.isFmiCredentialRequest = true;


Do we need an extra param here? Can't ManagedIdentity classes deal with this?

Sure, I can do that

Actually, it is better to keep this here because there the check is needed for the experimental features to be enabled.

I'm ok with dropping the experimental feature flag if it leads to better design.

Adding WithServiceFabricFmi()

7e29a8b

trwalke commented May 21, 2025

View reviewed changes

trwalke added 3 commits May 21, 2025 23:04

Moving OnBeforeTokenRequestHandler up one level

44e34c5

updating api

d1c9a99

Updating expiremental features

615e5d4

trwalke mentioned this pull request May 30, 2025

Create FmiCredential.md proposal #5309

Merged

1 task

trwalke added 5 commits May 30, 2025 02:15

Adding more logic to MSAL for FMI Credential

951943f

Revert "Moving OnBeforeTokenRequestHandler up one level"

784d438

This reverts commit 44e34c5.

Revert "updating api"

d60e236

This reverts commit d1c9a99.

Updating implementation

2b3c614

Updating api for FMI Mits

d14ee0c

trwalke changed the title ~~PROTOTYPE: Adding WithServiceFabricFmi()~~ PROTOTYPE: Adding FMI source to MI app Jun 6, 2025

Updating tests

2caf549

Adding experimental feature requirement.

trwalke changed the title ~~PROTOTYPE: Adding FMI source to MI app~~ Adding FMI source to MI app Jun 11, 2025

trwalke marked this pull request as ready for review June 11, 2025 07:24

trwalke requested a review from a team as a code owner June 11, 2025 07:24

bgavrilMS reviewed Jun 16, 2025

View reviewed changes

...client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForManagedIdentityParameterBuilder.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Jun 16, 2025

View reviewed changes

...t/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForManagedIdentityParameters.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Jun 16, 2025

View reviewed changes

...ent/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricFederatedManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Jun 16, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Jun 17, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs Outdated Show resolved Hide resolved

trwalke and others added 2 commits June 23, 2025 22:38

Refactoring

d3318fc

Clean up

Merge branch 'main' into trwalke/addFmiSFEndpoint

cf3f3bc

MZOLN reviewed Jun 24, 2025

View reviewed changes

trwalke added 2 commits June 25, 2025 14:17

Merge branch 'main' into trwalke/addFmiSFEndpoint

8d21f66

Merge branch 'main' into trwalke/addFmiSFEndpoint

62095ff

bgavrilMS requested review from neha-bhargava and gladjohn July 1, 2025 10:07

bgavrilMS reviewed Jul 1, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Jul 1, 2025

View reviewed changes

bgavrilMS requested changes Jul 1, 2025

View reviewed changes

Renaming fields/properties

5c1c4dd

bgavrilMS reviewed Jul 9, 2025

View reviewed changes

bgavrilMS requested a review from Robbie-Microsoft July 11, 2025 13:14

Adding FMI source to MI app #5299

Are you sure you want to change the base?

Adding FMI source to MI app #5299

Uh oh!

Conversation

trwalke commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trwalke May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trwalke commented May 21, 2025 •

edited

Loading

bgavrilMS May 21, 2025 •

edited

Loading

trwalke May 22, 2025 •

edited

Loading

bgavrilMS May 23, 2025 •

edited

Loading

bgavrilMS Jul 11, 2025 •

edited

Loading