-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Identity] ManagedIdentityCredential fix #13919
Changes from 2 commits
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 |
---|---|---|
|
@@ -196,7 +196,20 @@ export class ManagedIdentityCredential implements TokenCredential { | |
message: err.message | ||
}); | ||
|
||
// If either the network is unreachable, | ||
// we can safely assume the credential is unavailable. | ||
if (err.code === "ENETUNREACH") { | ||
const error = new CredentialUnavailable( | ||
"ManagedIdentityCredential is unavailable. Network unreachable." | ||
); | ||
|
||
logger.getToken.info(formatError(scopes, error)); | ||
throw error; | ||
} | ||
|
||
// If either the host was unreachable, | ||
// we can safely assume the credential is unavailable. | ||
if (err.code === "EHOSTUNREACH") { | ||
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. Do these map to unique 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 couldn't find any meaningful map between these codes and a statusCode in Node's source. Later if we find a clear map, we can clean this up 🌞 |
||
const error = new CredentialUnavailable( | ||
"ManagedIdentityCredential is unavailable. No managed identity endpoint found." | ||
); | ||
|
@@ -213,6 +226,15 @@ export class ManagedIdentityCredential implements TokenCredential { | |
); | ||
} | ||
|
||
// If the error has no status code, we can assume there was no available identity. | ||
// This will throw silently during any ChainedTokenCredential. | ||
if (err.statusCode === undefined) { | ||
throw new CredentialUnavailable( | ||
`ManagedIdentityCredential authentication failed. Message ${err.message}` | ||
); | ||
} | ||
|
||
// Any other error should break the chain. | ||
throw new AuthenticationError(err.statusCode, { | ||
error: "ManagedIdentityCredential authentication failed.", | ||
error_description: err.message | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,8 @@ import { | |
import * as coreHttp from "@azure/core-http"; | ||
|
||
export interface MockAuthResponse { | ||
status: number; | ||
status?: number; | ||
error?: RestError; | ||
headers?: HttpHeaders; | ||
parsedBody?: any; | ||
bodyAsText?: string; | ||
|
@@ -76,10 +77,19 @@ export class MockAuthHttpClient implements HttpClient { | |
throw new Error("The number of requests has exceeded the number of authResponses"); | ||
} | ||
|
||
const authResponse = this.authResponses[this.currentResponse]; | ||
|
||
if (authResponse.error) { | ||
this.currentResponse++; | ||
sadasant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw authResponse.error; | ||
} | ||
|
||
const response = { | ||
request: httpRequest, | ||
headers: this.authResponses[this.currentResponse].headers || new HttpHeaders(), | ||
...this.authResponses[this.currentResponse] | ||
headers: authResponse.headers || new HttpHeaders(), | ||
status: authResponse.status || 200, | ||
parsedBody: authResponse.parsedBody, | ||
bodyAsText: authResponse.bodyAsText | ||
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 was just to ease my mind. |
||
}; | ||
|
||
this.currentResponse++; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import { | |
imdsApiVersion | ||
} from "../../../src/credentials/managedIdentityCredential/constants"; | ||
import { MockAuthHttpClient, MockAuthHttpClientOptions, assertRejects } from "../../authTestUtils"; | ||
import { WebResource, AccessToken, HttpHeaders } from "@azure/core-http"; | ||
import { WebResource, AccessToken, HttpHeaders, RestError } from "@azure/core-http"; | ||
import { OAuthErrorResponse } from "../../../src/client/errors"; | ||
|
||
interface AuthRequestDetails { | ||
|
@@ -83,7 +83,24 @@ describe("ManagedIdentityCredential", function() { | |
} | ||
}); | ||
|
||
it("returns error when ManagedIdentityCredential authentication failed", async function() { | ||
it("returns error when no MSI is available", async function() { | ||
process.env.AZURE_CLIENT_ID = "errclient"; | ||
|
||
const imdsError: RestError = new RestError("Request Timeout", "REQUEST_SEND_ERROR", 408); | ||
const mockHttpClient = new MockAuthHttpClient({ | ||
authResponse: [{ error: imdsError }] | ||
}); | ||
|
||
const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { | ||
...mockHttpClient.tokenCredentialOptions | ||
}); | ||
await assertRejects( | ||
credential.getToken("scopes"), | ||
(error: AuthenticationError) => error.message.indexOf("No MSI credential available") > -1 | ||
); | ||
}); | ||
|
||
it("an unexpected error bubbles all the way up", async function() { | ||
process.env.AZURE_CLIENT_ID = "errclient"; | ||
|
||
const errResponse: OAuthErrorResponse = { | ||
|
@@ -92,7 +109,41 @@ describe("ManagedIdentityCredential", function() { | |
}; | ||
|
||
const mockHttpClient = new MockAuthHttpClient({ | ||
authResponse: [{ status: 400, parsedBody: errResponse }] | ||
authResponse: [{ status: 200 }, { status: 500, parsedBody: errResponse }] | ||
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. Since this test file was skipped, the old tests didn't match the utilities that were available, so I had to change them. I tried to make them better. Regarding this first 200 request, this determines that there's a MSI endpoint available. ManagedIdentityCredential expects at least one MSI approach to be available (generally speaking, this means at least one initial 200 response). |
||
}); | ||
|
||
const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { | ||
...mockHttpClient.tokenCredentialOptions | ||
}); | ||
await assertRejects( | ||
credential.getToken("scopes"), | ||
(error: AuthenticationError) => error.message.indexOf(errResponse.error) > -1 | ||
); | ||
}); | ||
|
||
it("returns expected error when the network was unreachable", async function() { | ||
process.env.AZURE_CLIENT_ID = "errclient"; | ||
|
||
const netError: RestError = new RestError("Request Timeout", "ENETUNREACH", 408); | ||
const mockHttpClient = new MockAuthHttpClient({ | ||
authResponse: [{ status: 200 }, { error: netError }] | ||
}); | ||
|
||
const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { | ||
...mockHttpClient.tokenCredentialOptions | ||
}); | ||
await assertRejects( | ||
credential.getToken("scopes"), | ||
(error: AuthenticationError) => error.message.indexOf("Network unreachable.") > -1 | ||
); | ||
}); | ||
|
||
it("returns expected error when the host was unreachable", async function() { | ||
process.env.AZURE_CLIENT_ID = "errclient"; | ||
|
||
const hostError: RestError = new RestError("Request Timeout", "EHOSTUNREACH", 408); | ||
const mockHttpClient = new MockAuthHttpClient({ | ||
authResponse: [{ status: 200 }, { error: hostError }] | ||
}); | ||
|
||
const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { | ||
|
@@ -101,7 +152,7 @@ describe("ManagedIdentityCredential", function() { | |
await assertRejects( | ||
credential.getToken("scopes"), | ||
(error: AuthenticationError) => | ||
error.errorResponse.error.indexOf("ManagedIdentityCredential authentication failed.") > -1 | ||
error.message.indexOf("No managed identity endpoint found.") > -1 | ||
); | ||
}); | ||
|
||
|
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.
Seems like we forgot to do this when we switched to this folder names!