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

[Identity] ManagedIdentityCredential fix #13919

Merged
4 commits merged into from
Feb 23, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
## 1.2.4-beta.2 (Unreleased)

- Breaking change: `DefaultAzureCredential` now only uses `InteractiveBrowserCredential` in the browser since it's the only credential intended to be used to authenticate within browsers.
- 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)

Expand Down
6 changes: 4 additions & 2 deletions sdk/identity/identity/rollup.base.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

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!

];
baseConfig.plugins.unshift(multiEntry({ exports: false }));

Expand Down Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Member

Choose a reason for hiding this comment

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

Do these map to unique statusCode values so we can stay consistent with the code below? Just curious!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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."
);
Expand All @@ -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
Expand Down
20 changes: 15 additions & 5 deletions sdk/identity/identity/test/authTestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just to ease my mind.

};

this.currentResponse++;
this.currentResponseIndex++;
return response;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 = {
Expand All @@ -92,7 +109,41 @@ describe("ManagedIdentityCredential", function() {
};

const mockHttpClient = new MockAuthHttpClient({
authResponse: [{ status: 400, parsedBody: errResponse }]
authResponse: [{ status: 200 }, { status: 500, parsedBody: errResponse }]
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, {
Expand All @@ -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
);
});

Expand Down