-
Notifications
You must be signed in to change notification settings - Fork 372
Adding FMI source to MI app #5299
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
base: main
Are you sure you want to change the base?
Conversation
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trwalke - can you put toghter a small design doc for this and let's discuss with the team?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgavrilMS Added here
#5309
Adding experimental feature requirement.
...client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForManagedIdentityParameterBuilder.cs
Outdated
Show resolved
Hide resolved
...t/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForManagedIdentityParameters.cs
Outdated
Show resolved
Hide resolved
...ent/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricFederatedManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
[TestMethod] | ||
public async Task ValidateThatFmiCredentialCanBeAcquiredFromMits() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a test where you ask for a FMI credential, then for an FMI token (and repeat).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source detection should not depend on isFmi
...client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForManagedIdentityParameterBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs
Show resolved
Hide resolved
Update, this work was put on hold as of July since a higher priority item needed to be completed. |
@@ -16,34 +16,86 @@ internal class ServiceFabricManagedIdentitySource : AbstractManagedIdentity | |||
private const string ServiceFabricMsiApiVersion = "2019-07-01-preview"; | |||
private readonly Uri _endpoint; | |||
private readonly string _identityHeaderValue; | |||
private readonly bool _isFmiCredentialRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend you park for now this or sync with @Robbie-Microsoft who's refactoring this completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
requestContext.Logger.Verbose(() => "[Managed Identity] Creating Service Fabric managed identity. Endpoint URI: " + identityEndpoint); | ||
|
||
return new ServiceFabricManagedIdentitySource(requestContext, endpointUri, EnvironmentVariables.IdentityHeader); | ||
return new ServiceFabricManagedIdentitySource(requestContext, endpointUri, EnvironmentVariables.IdentityHeader, isFmiCredentialRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YOu can have 1 request for FMI credential and 1 request for normal token. So this won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by this. The client will only do one or the other. The FMI version of this will only be used by MISE for the credential.
@Robbie-Microsoft - pls review this. |
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