From 39d7184595dbf977e47ac904e2ab3574ad1e85c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodr=C3=ADguez?= Date: Tue, 23 Feb 2021 15:55:02 -0500 Subject: [PATCH] [Identity] ManagedIdentityCredential fix (#13919) This pull request makes the ManagedIdentityCredential: - Treat unreachable host as we were treating unreachable network. - This should have been done before, but we didn't catch it. - I couldn't find any other error that seemed relevant to me, using this as reference: [link](https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe019a8b75bfa04c3f/deps/uv/src/win/error.c). - Treat errors with undefined status code as if the credential was unavailable, to avoid breaking the chained token credentials in that case. - Add tests (turns out that some where being skipped by mistake!) - Add more comments to improve reading. This PR: Fixes #13894 --- sdk/identity/identity/CHANGELOG.md | 1 + sdk/identity/identity/rollup.base.config.js | 6 +- .../managedIdentityCredential/index.ts | 22 +++++++ sdk/identity/identity/test/authTestUtils.ts | 20 +++++-- .../node/managedIdentityCredential.spec.ts | 59 +++++++++++++++++-- 5 files changed, 97 insertions(+), 11 deletions(-) diff --git a/sdk/identity/identity/CHANGELOG.md b/sdk/identity/identity/CHANGELOG.md index 91045a5b928..3de3e50242e 100644 --- a/sdk/identity/identity/CHANGELOG.md +++ b/sdk/identity/identity/CHANGELOG.md @@ -7,6 +7,7 @@ - `DefaultAzureCredential`'s implementation for browsers was simplified to throw a simple error instead of trying credentials that were already not supported for the browser. - Breaking Change: `InteractiveBrowserCredential` for the browser now requires the client ID to be provided. - Documentation was added to elaborate on how to configure an AAD application to support `InteractiveBrowserCredential`. +- Bug fix: `ManagedIdentityCredential` now also properly handles `EHOSTUNREACH` errors. Fixes issue [13894](https://github.com/Azure/azure-sdk-for-js/issues/13894). ## 1.2.4-beta.1 (2021-02-12) diff --git a/sdk/identity/identity/rollup.base.config.js b/sdk/identity/identity/rollup.base.config.js index 763201886cf..444bea458f1 100644 --- a/sdk/identity/identity/rollup.base.config.js +++ b/sdk/identity/identity/rollup.base.config.js @@ -38,7 +38,8 @@ export function nodeConfig(test = false) { baseConfig.input = [ "dist-esm/test/public/*.spec.js", "dist-esm/test/public/node/*.spec.js", - "dist-esm/test/internal/*.spec.js" + "dist-esm/test/internal/*.spec.js", + "dist-esm/test/internal/node/*.spec.js" ]; baseConfig.plugins.unshift(multiEntry({ exports: false })); @@ -97,7 +98,8 @@ export function browserConfig(test = false) { baseConfig.input = [ "dist-esm/test/public/*.spec.js", "dist-esm/test/public/browser/*.spec.js", - "dist-esm/test/internal/*.spec.js" + "dist-esm/test/internal/*.spec.js", + "dist-esm/test/internal/browser/*.spec.js" ]; baseConfig.plugins.unshift(multiEntry({ exports: false })); baseConfig.output.file = "test-browser/index.js"; diff --git a/sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts b/sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts index 58ecfef907a..1337b188b51 100644 --- a/sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts +++ b/sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts @@ -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") { 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 diff --git a/sdk/identity/identity/test/authTestUtils.ts b/sdk/identity/identity/test/authTestUtils.ts index 068a3d9102f..d60dea1d123 100644 --- a/sdk/identity/identity/test/authTestUtils.ts +++ b/sdk/identity/identity/test/authTestUtils.ts @@ -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; @@ -27,7 +28,7 @@ export interface MockAuthHttpClientOptions { export class MockAuthHttpClient implements HttpClient { private authResponses: MockAuthResponse[] = []; - private currentResponse: number = 0; + private currentResponseIndex: number = 0; private mockTimeout: boolean; public tokenCredentialOptions: ClientCertificateCredentialOptions; @@ -76,13 +77,22 @@ export class MockAuthHttpClient implements HttpClient { throw new Error("The number of requests has exceeded the number of authResponses"); } + const authResponse = this.authResponses[this.currentResponseIndex]; + + if (authResponse.error) { + this.currentResponseIndex++; + 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 }; - this.currentResponse++; + this.currentResponseIndex++; return response; } } diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index ca8d663fdc3..0a1819183bf 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -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 }] + }); + + 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 ); });