Skip to content

Commit

Permalink
Fixed incorrect IMDS resource ID query parameter (#7488)
Browse files Browse the repository at this point in the history 
Fixes #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](https://github.com/Azure/azure-rest-api-specs/blob/dba6ed1f03bda88ac6884c0a883246446cc72495/specification/imds/data-plane/Microsoft.InstanceMetadataService/stable/2018-10-01/imds.json#L233-L239).

> Original issue:

AzureAD/microsoft-authentication-library-for-dotnet#4911
  • Loading branch information
@Robbie-Microsoft
Robbie-Microsoft authored Jan 15, 2025
1 parent 69238d1 commit d280859
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 37 deletions.
7 changes: 7 additions & 0 deletions change/@azure-msal-node-643086d5-3524-43f3-adf2-97b752020dda.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fixed incorrect IMDS resource ID query parameter",
"packageName": "@azure/msal-node",
"email": "[email protected]",
"dependentChangeType": "patch"
}
10 changes: 7 additions & 3 deletions lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ 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_IMDS: "msi_res_id",
MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS: "mi_res_id",
} as const;
export type ManagedIdentityUserAssignedIdQueryParameterNames =
(typeof ManagedIdentityUserAssignedIdQueryParameterNames)[keyof typeof ManagedIdentityUserAssignedIdQueryParameterNames];
Expand Down Expand Up @@ -201,7 +202,8 @@ export abstract class BaseManagedIdentitySource {
}

public getManagedIdentityUserAssignedIdQueryParameterKey(
managedIdentityIdType: ManagedIdentityIdType
managedIdentityIdType: ManagedIdentityIdType,
imds?: boolean
): string {
switch (managedIdentityIdType) {
case ManagedIdentityIdType.USER_ASSIGNED_CLIENT_ID:
Expand All @@ -214,7 +216,9 @@ export abstract class BaseManagedIdentitySource {
this.logger.info(
"[Managed Identity] Adding user assigned resource id to the request."
);
return ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID;
return imds
? ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS
: ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS;

case ManagedIdentityIdType.USER_ASSIGNED_OBJECT_ID:
this.logger.info(
Expand Down
3 changes: 2 additions & 1 deletion lib/msal-node/src/client/ManagedIdentitySources/Imds.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ export class Imds extends BaseManagedIdentitySource {
) {
request.queryParameters[
this.getManagedIdentityUserAssignedIdQueryParameterKey(
managedIdentityId.idType
managedIdentityId.idType,
true // indicates source is IMDS
)
] = managedIdentityId.id;
}
Expand Down
67 changes: 55 additions & 12 deletions lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT,
MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR,
MANAGED_IDENTITY_RESOURCE,
MANAGED_IDENTITY_RESOURCE_ID,
} from "../../test_kit/StringConstants.js";

import {
Expand All @@ -17,6 +18,7 @@ import {
systemAssignedConfig,
networkClient,
ManagedIdentityNetworkErrorClient,
userAssignedResourceIdConfig,
} from "../../test_kit/ManagedIdentityTestUtils.js";
import {
AuthenticationResult,
Expand All @@ -28,6 +30,7 @@ import {
ManagedIdentityEnvironmentVariableNames,
ManagedIdentitySourceNames,
} from "../../../src/utils/Constants.js";
import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js";

describe("Acquires a token successfully via an App Service Managed Identity", () => {
beforeAll(() => {
Expand All @@ -52,21 +55,61 @@ describe("Acquires a token successfully via an App Service Managed Identity", ()
delete ManagedIdentityApplication["nodeStorage"];
});

test("acquires a User Assigned Client Id token", async () => {
const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedClientIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.APP_SERVICE
);
describe("User Assigned", () => {
test("acquires a User Assigned Client Id token", async () => {
const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedClientIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.APP_SERVICE
);

const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
);

const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
});

test("acquires a User Assigned Resource Id token", async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedResourceIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.APP_SERVICE
);

expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
);

expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

const url: URLSearchParams = new URLSearchParams(
sendGetRequestAsyncSpy.mock.lastCall[0]
);
expect(
url.has(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS
)
).toBe(true);
expect(
url.get(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS
)
).toEqual(MANAGED_IDENTITY_RESOURCE_ID);

jest.restoreAllMocks();
});
});

describe("System Assigned", () => {
Expand Down
32 changes: 23 additions & 9 deletions lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import {
THREE_SECONDS_IN_MILLI,
getCacheKey,
} from "../../test_kit/StringConstants.js";

import {
ManagedIdentityNetworkClient,
ManagedIdentityNetworkErrorClient,
networkClient,
userAssignedClientIdConfig,
managedIdentityRequestParams,
systemAssignedConfig,
userAssignedResourceIdConfig,
} from "../../test_kit/ManagedIdentityTestUtils.js";
import {
DEFAULT_MANAGED_IDENTITY_ID,
Expand Down Expand Up @@ -53,6 +53,7 @@ import { setTimeout } from "timers/promises";
import { ClientCredentialClient } from "../../../src/client/ClientCredentialClient.js";
import { NodeStorage } from "../../../src/cache/NodeStorage.js";
import { CacheKVStore } from "../../../src/cache/serializer/SerializerTypes.js";
import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js";

describe("Acquires a token successfully via an IMDS Managed Identity", () => {
// IMDS doesn't need environment variables because there is a default IMDS endpoint
Expand All @@ -79,14 +80,6 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
userAssignedObjectId: MANAGED_IDENTITY_RESOURCE_ID,
},
};
const userAssignedResourceIdConfig: ManagedIdentityConfiguration = {
system: {
networkClient,
},
managedIdentityIdParams: {
userAssignedResourceId: MANAGED_IDENTITY_RESOURCE_ID,
},
};

describe("User Assigned", () => {
test("acquires a User Assigned Client Id token", async () => {
Expand Down Expand Up @@ -124,6 +117,11 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
});

test("acquires a User Assigned Resource Id token", async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedResourceIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
Expand All @@ -138,6 +136,22 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

const url: URLSearchParams = new URLSearchParams(
sendGetRequestAsyncSpy.mock.lastCall[0]
);
expect(
url.has(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS
)
).toBe(true);
expect(
url.get(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_IMDS
)
).toEqual(MANAGED_IDENTITY_RESOURCE_ID);

jest.restoreAllMocks();
});
});

Expand Down
66 changes: 54 additions & 12 deletions lib/msal-node/test/client/ManagedIdentitySources/ServiceFabric.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT,
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT,
MANAGED_IDENTITY_RESOURCE,
MANAGED_IDENTITY_RESOURCE_ID,
MANAGED_IDENTITY_SERVICE_FABRIC_NETWORK_REQUEST_400_ERROR,
} from "../../test_kit/StringConstants.js";

Expand All @@ -17,6 +18,7 @@ import {
systemAssignedConfig,
ManagedIdentityNetworkErrorClient,
networkClient,
userAssignedResourceIdConfig,
} from "../../test_kit/ManagedIdentityTestUtils.js";
import {
AuthenticationResult,
Expand All @@ -28,6 +30,7 @@ import {
ManagedIdentityEnvironmentVariableNames,
ManagedIdentitySourceNames,
} from "../../../src/utils/Constants.js";
import { ManagedIdentityUserAssignedIdQueryParameterNames } from "../../../src/client/ManagedIdentitySources/BaseManagedIdentitySource.js";

describe("Acquires a token successfully via an App Service Managed Identity", () => {
beforeAll(() => {
Expand Down Expand Up @@ -58,21 +61,60 @@ describe("Acquires a token successfully via an App Service Managed Identity", ()
delete ManagedIdentityApplication["nodeStorage"];
});

test("acquires a User Assigned Client Id token", async () => {
const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedClientIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.SERVICE_FABRIC
);
describe("User Assigned", () => {
test("acquires a User Assigned Client Id token", async () => {
const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedClientIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.SERVICE_FABRIC
);

const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
);

expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
});
test("acquires a User Assigned Resource Id token", async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

const managedIdentityApplication: ManagedIdentityApplication =
new ManagedIdentityApplication(userAssignedResourceIdConfig);
expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
ManagedIdentitySourceNames.SERVICE_FABRIC
);

const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
const networkManagedIdentityResult: AuthenticationResult =
await managedIdentityApplication.acquireToken(
managedIdentityRequestParams
);

expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
const url: URLSearchParams = new URLSearchParams(
sendGetRequestAsyncSpy.mock.lastCall[0]
);
expect(
url.has(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS
)
).toBe(true);
expect(
url.get(
ManagedIdentityUserAssignedIdQueryParameterNames.MANAGED_IDENTITY_RESOURCE_ID_NON_IMDS
)
).toEqual(MANAGED_IDENTITY_RESOURCE_ID);

jest.restoreAllMocks();
});
});

describe("System Assigned", () => {
Expand Down
9 changes: 9 additions & 0 deletions lib/msal-node/test/test_kit/ManagedIdentityTestUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ export const userAssignedClientIdConfig: ManagedIdentityConfiguration = {
},
};

export const userAssignedResourceIdConfig: ManagedIdentityConfiguration = {
system: {
networkClient,
},
managedIdentityIdParams: {
userAssignedResourceId: MANAGED_IDENTITY_RESOURCE_ID,
},
};

export const systemAssignedConfig: ManagedIdentityConfiguration = {
system: {
networkClient,
Expand Down

0 comments on commit d280859

Please sign in to comment.