Skip to content
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

Fixed incorrect IMDS resource ID query parameter #7488

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Robbie-Microsoft
Copy link
Collaborator

@Robbie-Microsoft Robbie-Microsoft commented Jan 2, 2025

#7487

Specifying an identity by resource ID doesn't work on Azure Container Instances (and maybe some other platforms) because we specify the resource ID with query parameter "mi_res_id". ACI observes only "msi_res_id", which we should prefer anyway because although IMDS observes both parameters, it documents only msi_res_id.

Original issue:
AzureAD/microsoft-authentication-library-for-dotnet#4911

Tests don't need to be updated.

@github-actions github-actions bot added the msal-node Related to msal-node package label Jan 2, 2025
@Robbie-Microsoft Robbie-Microsoft linked an issue Jan 2, 2025 that may be closed by this pull request
@@ -38,7 +38,7 @@ import {
export const ManagedIdentityUserAssignedIdQueryParameterNames = {
MANAGED_IDENTITY_CLIENT_ID: "client_id",
MANAGED_IDENTITY_OBJECT_ID: "object_id",
MANAGED_IDENTITY_RESOURCE_ID: "mi_res_id",
MANAGED_IDENTITY_RESOURCE_ID: "msi_res_id",
Copy link
Member

Choose a reason for hiding this comment

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

The bug states that this change should only be done for IMDS, not for all sources.

Copy link
Member

Choose a reason for hiding this comment

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

Also, pls take the time to improve your tests. The fact that you make a change like this and no test breaks is sign that code coverage can be improved.

Copy link

Choose a reason for hiding this comment

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

Isn't this change desirable to more than just IMDS?

Copy link
Member

Choose a reason for hiding this comment

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

It seems that only IMDS documents this as "msi_res_id" and the rest do not. App service does not. Not sure about the others.

Copy link

Choose a reason for hiding this comment

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

Actually, for "the others", only Machine Learning is applicable, and we assume it follows IMDS's "msi_res_id", too.

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.

Only IMDS needs this.

@Robbie-Microsoft Robbie-Microsoft marked this pull request as draft January 2, 2025 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct IMDS resource ID query parameter
3 participants