-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Added token revocation functionality to Managed Identity's App Service and Service Fabric sources #7679
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: dev
Are you sure you want to change the base?
Added token revocation functionality to Managed Identity's App Service and Service Fabric sources #7679
Changes from 9 commits
920bac3
73ffd22
f4f002a
468de01
dcd382f
d8fbbca
b09a464
0f7b2b9
b32f45e
604194d
3405bce
f1d096f
203d5d0
d9f15e6
0832c02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "minor", | ||
"comment": "Added token revocation functionality to Managed Identity's App Service and Service Fabric Sources #7679", | ||
"packageName": "@azure/msal-node", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,13 @@ import { | |
DEFAULT_AUTHORITY_FOR_MANAGED_IDENTITY, | ||
ManagedIdentitySourceNames, | ||
} from "../utils/Constants.js"; | ||
import { ManagedIdentityId } from "../config/ManagedIdentityId.js"; | ||
|
||
const SOURCES_THAT_SUPPORT_TOKEN_REVOCATION: Array<ManagedIdentitySourceNames> = | ||
[ | ||
ManagedIdentitySourceNames.APP_SERVICE, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove app service, you can merge the PR. we can add app service later |
||
ManagedIdentitySourceNames.SERVICE_FABRIC, | ||
]; | ||
|
||
/** | ||
* Class to initialize a managed identity and identify the service | ||
|
@@ -141,14 +148,12 @@ export class ManagedIdentityApplication { | |
], | ||
authority: this.fakeAuthority.canonicalAuthority, | ||
correlationId: this.cryptoProvider.createNewGuid(), | ||
claims: managedIdentityRequestParams.claims, | ||
clientCapabilities: this.config.clientCapabilities, | ||
}; | ||
|
||
if ( | ||
managedIdentityRequestParams.claims || | ||
managedIdentityRequest.forceRefresh | ||
) { | ||
// make a network call to the managed identity source | ||
return this.managedIdentityClient.sendManagedIdentityTokenRequest( | ||
if (managedIdentityRequest.forceRefresh) { | ||
return this.acquireTokenFromManagedIdentity( | ||
managedIdentityRequest, | ||
this.config.managedIdentityId, | ||
this.fakeAuthority | ||
|
@@ -164,16 +169,47 @@ export class ManagedIdentityApplication { | |
ManagedIdentityApplication.nodeStorage as NodeStorage | ||
); | ||
|
||
/* | ||
* Check if claims are present in the managed identity request. | ||
* If so, the cached token will not be used. | ||
*/ | ||
if (managedIdentityRequest.claims) { | ||
const sourceName: ManagedIdentitySourceNames = | ||
this.managedIdentityClient.getManagedIdentitySource(); | ||
|
||
/* | ||
* Check if there is a cached token and if the Managed Identity source supports token revocation. | ||
* If so, hash the cached access token and add it to the request. | ||
*/ | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GOOD - if claims are set and you have a cached token, you skip the cache for that request, if the source supports revocation, you also send the old token’s SHA-256 to the server. This ensures the old token is invalidated on the RP |
||
cachedAuthenticationResult && | ||
SOURCES_THAT_SUPPORT_TOKEN_REVOCATION.includes(sourceName) | ||
) { | ||
const revokedTokenSha256Hash: string = | ||
await this.cryptoProvider.hashString( | ||
cachedAuthenticationResult.accessToken | ||
); | ||
managedIdentityRequest.revokedTokenSha256Hash = | ||
revokedTokenSha256Hash; | ||
} | ||
|
||
return this.acquireTokenFromManagedIdentity( | ||
managedIdentityRequest, | ||
this.config.managedIdentityId, | ||
this.fakeAuthority | ||
); | ||
} | ||
|
||
if (cachedAuthenticationResult) { | ||
// if the token is not expired but must be refreshed; get a new one in the background | ||
if (lastCacheOutcome === CacheOutcome.PROACTIVELY_REFRESHED) { | ||
this.logger.info( | ||
"ClientCredentialClient:getCachedAuthenticationResult - Cached access token's refreshOn property has been exceeded'. It's not expired, but must be refreshed." | ||
); | ||
|
||
// make a network call to the managed identity source; refresh the access token in the background | ||
// force refresh; will run in the background | ||
const refreshAccessToken = true; | ||
await this.managedIdentityClient.sendManagedIdentityTokenRequest( | ||
await this.acquireTokenFromManagedIdentity( | ||
managedIdentityRequest, | ||
this.config.managedIdentityId, | ||
this.fakeAuthority, | ||
|
@@ -183,15 +219,39 @@ export class ManagedIdentityApplication { | |
|
||
return cachedAuthenticationResult; | ||
} else { | ||
// make a network call to the managed identity source | ||
return this.managedIdentityClient.sendManagedIdentityTokenRequest( | ||
return this.acquireTokenFromManagedIdentity( | ||
managedIdentityRequest, | ||
this.config.managedIdentityId, | ||
this.fakeAuthority | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Acquires a token from a managed identity endpoint. | ||
* | ||
* @param managedIdentityRequest - The request object containing parameters for the managed identity token request. | ||
* @param managedIdentityId - The identifier for the managed identity (e.g., client ID or resource ID). | ||
* @param fakeAuthority - A placeholder authority used for the token request. | ||
* @param refreshAccessToken - Optional flag indicating whether to force a refresh of the access token. | ||
* @returns A promise that resolves to an {@link AuthenticationResult} containing the acquired token and related information. | ||
* @throws {@link AuthenticationError} if the token acquisition fails. | ||
*/ | ||
private async acquireTokenFromManagedIdentity( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this method needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extracted the functionality for the network request since it's being used multiple times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allowed me to write better docs for it |
||
managedIdentityRequest: ManagedIdentityRequest, | ||
managedIdentityId: ManagedIdentityId, | ||
fakeAuthority: Authority, | ||
refreshAccessToken?: boolean | ||
): Promise<AuthenticationResult> { | ||
// make a network call to the managed identity | ||
return this.managedIdentityClient.sendManagedIdentityTokenRequest( | ||
managedIdentityRequest, | ||
managedIdentityId, | ||
fakeAuthority, | ||
refreshAccessToken | ||
); | ||
} | ||
|
||
/** | ||
* Determine the Managed Identity Source based on available environment variables. This API is consumed by Azure Identity SDK. | ||
* @returns ManagedIdentitySourceNames - The Managed Identity source's name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,20 +7,19 @@ import { INetworkModule, Logger } from "@azure/msal-common/node"; | |
import { BaseManagedIdentitySource } from "./BaseManagedIdentitySource.js"; | ||
import { | ||
HttpMethod, | ||
APP_SERVICE_SECRET_HEADER_NAME, | ||
API_VERSION_QUERY_PARAMETER_NAME, | ||
RESOURCE_BODY_OR_QUERY_PARAMETER_NAME, | ||
ManagedIdentityEnvironmentVariableNames, | ||
ManagedIdentitySourceNames, | ||
ManagedIdentityIdType, | ||
ManagedIdentityQueryParameters, | ||
ManagedIdentityHeaders, | ||
} from "../../utils/Constants.js"; | ||
import { CryptoProvider } from "../../crypto/CryptoProvider.js"; | ||
import { ManagedIdentityRequestParameters } from "../../config/ManagedIdentityRequestParameters.js"; | ||
import { ManagedIdentityId } from "../../config/ManagedIdentityId.js"; | ||
import { NodeStorage } from "../../cache/NodeStorage.js"; | ||
|
||
// MSI Constants. Docs for MSI are available here https://docs.microsoft.com/azure/app-service/overview-managed-identity | ||
const APP_SERVICE_MSI_API_VERSION: string = "2019-08-01"; | ||
const APP_SERVICE_MSI_API_VERSION: string = "2025-03-30"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please do not merge this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, remove the app service changes from this PR, and then we can merge it for Service Fabric. We will create a new PR later after app service deploys their token revocation support. |
||
|
||
/** | ||
* Original source of code: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/identity/Azure.Identity/src/AppServiceManagedIdentitySource.cs | ||
|
@@ -114,11 +113,12 @@ export class AppService extends BaseManagedIdentitySource { | |
this.identityEndpoint | ||
); | ||
|
||
request.headers[APP_SERVICE_SECRET_HEADER_NAME] = this.identityHeader; | ||
request.headers[ManagedIdentityHeaders.APP_SERVICE_SECRET_HEADER_NAME] = | ||
this.identityHeader; | ||
|
||
request.queryParameters[API_VERSION_QUERY_PARAMETER_NAME] = | ||
request.queryParameters[ManagedIdentityQueryParameters.API_VERSION] = | ||
APP_SERVICE_MSI_API_VERSION; | ||
request.queryParameters[RESOURCE_BODY_OR_QUERY_PARAMETER_NAME] = | ||
request.queryParameters[ManagedIdentityQueryParameters.RESOURCE] = | ||
resource; | ||
|
||
if ( | ||
|
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.
Great that you have a dedicated list of sources that handle revocation. If more services adopt it, it should be trivial to add them to SOURCES_THAT_SUPPORT_TOKEN_REVOCATION.
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.
It won't be that easy, would it? Each MSIv1 provider is different. We will need to know their API version (which may or may not change, and if they do, other aspects of the protocol will likely also change).
(I am not necessarily against the current implementation, as long as it is functionally correct. I am just calling out the need to still be careful when more services adopt token revocation.)