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

Added token revocation functionality to Managed Identity #7422

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Added token revocation functionality to managed identity (#7422)",
"packageName": "@azure/msal-node",
"email": "[email protected]",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions lib/msal-node/apiReview/msal-node.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ export class ManagedIdentityApplication {

// @public (undocumented)
export type ManagedIdentityConfiguration = {
clientCapabilities?: Array<string>;
managedIdentityIdParams?: ManagedIdentityIdParams;
system?: NodeSystemOptions;
};
Expand Down
4 changes: 4 additions & 0 deletions lib/msal-node/src/client/ManagedIdentityApplication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ export class ManagedIdentityApplication {
],
authority: this.fakeAuthority.canonicalAuthority,
correlationId: this.cryptoProvider.createNewGuid(),
claims: managedIdentityRequestParams.claims,
clientCapabilities:
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
managedIdentityRequestParams.clientCapabilities ||
this.config.clientCapabilities,
};

if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,17 @@ export abstract class BaseManagedIdentitySource {
managedIdentityId
);

// if claims are present, ESTS will get a new token
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
if (managedIdentityRequest.claims) {

Choose a reason for hiding this comment

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

suggest using const here,

const BYPASS_CACHE = "bypass_cache";
const XMS_CC = "xms_cc";

Choose a reason for hiding this comment

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

can we add logs to these two conditions? i.e. when claims or capabilities are present?

networkRequest.queryParameters.bypass_cache = "true";
}

// if client capabilities are present, send them to ESTS
if (managedIdentityRequest.clientCapabilities?.length) {
networkRequest.queryParameters.xms_cc =
managedIdentityRequest.clientCapabilities.toString();
}

const headers: Record<string, string> = networkRequest.headers;
headers[HeaderNames.CONTENT_TYPE] = Constants.URL_FORM_CONTENT_TYPE;

Expand Down
4 changes: 4 additions & 0 deletions lib/msal-node/src/config/Configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export type ManagedIdentityIdParams = {

/** @public */
export type ManagedIdentityConfiguration = {
clientCapabilities?: Array<string>;
managedIdentityIdParams?: ManagedIdentityIdParams;
system?: NodeSystemOptions;
};
Expand Down Expand Up @@ -247,13 +248,15 @@ export function buildAppConfiguration({

/** @internal */
export type ManagedIdentityNodeConfiguration = {
clientCapabilities: Array<string>;
managedIdentityId: ManagedIdentityId;
system: Required<
Pick<NodeSystemOptions, "loggerOptions" | "networkClient">
>;
};

export function buildManagedIdentityConfiguration({
clientCapabilities,
managedIdentityIdParams,
system,
}: ManagedIdentityConfiguration): ManagedIdentityNodeConfiguration {
Expand Down Expand Up @@ -290,6 +293,7 @@ export function buildManagedIdentityConfiguration({
}

return {
clientCapabilities: clientCapabilities || [],
managedIdentityId: managedIdentityId,
system: {
loggerOptions,
Expand Down
8 changes: 5 additions & 3 deletions lib/msal-node/src/request/ManagedIdentityRequestParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@

/**
* ManagedIdentityRequest
* - claims - a stringified claims request which will be used to determine whether or not the cache should be skipped
* - forceRefresh - forces managed identity requests to skip the cache and make network calls if true
* - resource - resource requested to access the protected API. It should be of the form "ResourceIdUri" or "ResourceIdUri/.default". For instance https://management.azure.net or, for Microsoft Graph, https://graph.microsoft.com/.default
* - claims - a stringified claims request which will be used to determine whether or not the cache should be skipped
* - clientCapabilities - an array of client capabilities to be sent to ESTS
* - forceRefresh - forces managed identity requests to skip the cache and make network calls if true
* - resource - resource requested to access the protected API. It should be of the form "ResourceIdUri" or "ResourceIdUri/.default". For instance https://management.azure.net or, for Microsoft Graph, https://graph.microsoft.com/.default
* @public
*/
export type ManagedIdentityRequestParams = {
claims?: string;
clientCapabilities?: Array<string>;
forceRefresh?: boolean;
resource: string;
};
130 changes: 129 additions & 1 deletion lib/msal-node/test/client/ManagedIdentitySources/Imds.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
} from "../../../src";
Copy link
Member

Choose a reason for hiding this comment

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

The tests aren't related to IMDS? Is this the right location for them?

// NodeJS 16+ provides a built-in version of setTimeout that is promise-based
import { setTimeout } from "timers/promises";
import { CAE_CONSTANTS } from "../../test_kit/StringConstants.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 Down Expand Up @@ -549,13 +550,136 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
);
});

test("ignores a cached token when claims are provided", async () => {
test('checks if a token was cached or not and if was "bypass_cache=true" was sent to ESTS depending on whether or not claims are provided, and ensures "xms=client-capabilites-string" was sent to ESTS when client capabilities are provided via the Managed Identity Request Parameters', async () => {

Choose a reason for hiding this comment

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

I think you should also include tests when claims are passed, and client capabilities are not and when client capabilities are passed, and claims are not.

Copy link
Member

@bgavrilMS bgavrilMS Nov 26, 2024

Choose a reason for hiding this comment

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

I think some are covered, but the tests are somewhat difficult to read.

There are 3 dimensions for this test, each with 2 possible values. So a total of 2^3 = 8 combos.

CP1: {sent, not_sent}
Claims: {sent, not_sent}
Token_in_cache? { yes, no}

const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
let tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
let url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(false);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
const cachedManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(cachedManagedIdentityResult.fromCache).toBe(true);
expect(cachedManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

Robbie-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
networkManagedIdentityResult =
await systemAssignedManagedIdentityApplication.acquireToken({
claims: TEST_CONFIG.CLAIMS,
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);
});

test('checks if a token was cached or not and if was "bypass_cache=true" was sent to ESTS depending on whether or not claims are provided, and ensures "xms=client-capabilites-string" was sent to ESTS when client capabilities are provided via the Managed Identity configuration object', async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

const systemAssignedManagedIdentityApplicationWithClientCapabilities: ManagedIdentityApplication =
new ManagedIdentityApplication({
...systemAssignedConfig,
clientCapabilities: CAE_CONSTANTS.CLIENT_CAPABILITIES,
});

let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

let tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
let url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(false);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);

const cachedManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(cachedManagedIdentityResult.fromCache).toBe(true);
expect(cachedManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

networkManagedIdentityResult =
await systemAssignedManagedIdentityApplicationWithClientCapabilities.acquireToken(
{
claims: TEST_CONFIG.CLAIMS,
resource: MANAGED_IDENTITY_RESOURCE,
}
);
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
expect(
url.includes(
`xms_cc=${CAE_CONSTANTS.CLIENT_CAPABILITIES.toString()}`
)
).toBe(true);
});

test('ignores a cached token when claims are provided, and ensures "bypass_cache=true" was sent to ESTS', async () => {
const sendGetRequestAsyncSpy: jest.SpyInstance = jest.spyOn(
networkClient,
<any>"sendGetRequestAsync"
);

let networkManagedIdentityResult: AuthenticationResult =
await systemAssignedManagedIdentityApplication.acquireToken({
resource: MANAGED_IDENTITY_RESOURCE,
});
expect(networkManagedIdentityResult.fromCache).toBe(false);
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);
Expand All @@ -578,6 +702,10 @@ describe("Acquires a token successfully via an IMDS Managed Identity", () => {
expect(networkManagedIdentityResult.accessToken).toEqual(
DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT.accessToken
);

const tokenRequest = sendGetRequestAsyncSpy.mock.lastCall;
const url = tokenRequest[0];
expect(url.includes("bypass_cache=true")).toBe(true);
});

test("ignores a cached token when forceRefresh is set to true", async () => {
Expand Down
Loading