[Identity] Support exchanging k8s token to AAD token #16688

sadasant · 2021-07-31T00:04:44Z

This is a simplified version of what @chlowell did on this PR: Azure/azure-sdk-for-python#19902

This is based on what I understood. I’ll make sure to circle back with Charles before I get this PR out of draft.

sadasant · 2021-09-01T01:00:00Z

Hello, @chlowell , @schaabs help me review this. We can get on a call. I took a different approach than Python, but the effect should be the same.

sadasant · 2021-09-01T01:27:59Z

I took this out of draft before fixing conflicts. One moment.

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenFilePathMsi.ts

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

sadasant · 2021-09-02T18:00:53Z

sdk/identity/identity/CHANGELOG.md

@@ -4,6 +4,8 @@

 ### Features Added

+- `ManagedIdentityCredential` now supports token exchange authentication.


This is intentionally vague.

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

… error messages

sadasant · 2021-09-02T22:16:47Z

@chlowell ok now I’ve tested this in the real test environment, and this works 😃

sadasant · 2021-09-02T23:24:35Z

I’m investigating the CI issue.

sadasant · 2021-09-03T00:46:26Z

I don’t quite get what’s going on on CI. Tomorrow is a new day!

sadasant · 2021-09-03T17:28:16Z

sdk/identity/identity/src/credentials/chainedTokenCredential.ts

+      const err = new AggregateAuthenticationError(
+        errors,
+        "ChainedTokenCredential authentication failed."
+      );


Without this, this error had an initial segment with an undefined string. This PR adds a test to ensure this is never silently broken in the future.

….. might fix CI

…his new MSI, and improve MSI logs

sadasant · 2021-09-04T00:01:36Z

sdk/identity/identity/src/client/errors.ts

@@ -164,7 +164,7 @@ export class AggregateAuthenticationError extends Error {

  constructor(errors: any[], errorMessage?: string) {
    const errorDetail = errors.join("\n");
-    super(`${errorMessage}\n\n${errorDetail}`);
+    super(`${errorMessage}\n${errorDetail}`);


This meant a whole empty line, which I didn’t like now that I re-reviewed these errors.

sadasant · 2021-09-04T00:03:52Z

sdk/identity/identity/src/credentials/managedIdentityCredential/index.ts

@@ -214,7 +223,7 @@ export class ManagedIdentityCredential implements TokenCredential {
      // and it means that the endpoint is working, but that no identity is available.
      if (err.statusCode === 400) {
        throw new CredentialUnavailableError(
-          "The managed identity endpoint is indicating there's no available identity"
+          `ManagedIdentityCredential: The managed identity endpoint is indicating there's no available identity. Message: ${err.message}`


This was the root of the CI issue: The changes in this PR made it so now this credential only failed due to the IMDS MSI on CI. Since CI is in Azure, the IMDS endpoint is reachable, so the execution reaches this statement, and this statement didn’t match the format we had for all of the other errors in this credential.

sadasant · 2021-09-04T00:37:31Z

Oh, the taste of a green CI build 😌💚

sdk/identity/identity/src/credentials/managedIdentityCredential/models.ts

sdk/identity/identity/src/credentials/managedIdentityCredential/utils.ts

…h multiple resources, and an internal cleanup was in order (including slightly better logs and errors

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts

chlowell · 2021-09-08T01:07:21Z

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

+
+    const authDetails = await sendCredentialRequests({
+      scopes: ["https://service/.default"],
+      credential: new ManagedIdentityCredential("client"),


Do you have a test for the parameter overriding AZURE_CLIENT_ID?

I’m adding one! Thanks!

…l/tokenExchangeMsi.ts Co-authored-by: Charles Lowell <chlowe@microsoft.com>

chlowell

Token exchange logic looks good to me 👍

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts

ghost · 2021-09-08T16:47:15Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

[Identity] Support exchanging k8s token to AAD token

9d396d3

sadasant requested a review from chlowell July 31, 2021 00:04

sadasant self-assigned this Jul 31, 2021

ghost added the Azure.Identity label Jul 31, 2021

sadasant requested a review from schaabs September 1, 2021 00:59

sadasant marked this pull request as ready for review September 1, 2021 01:20

sadasant requested a review from witemple-msft as a code owner September 1, 2021 01:20

Merge remote-tracking branch 'Azure/main' into identity/fix15800

684c32e

fixes after merging main

5dede8c

chlowell reviewed Sep 1, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenFilePathMsi.ts Outdated Show resolved Hide resolved

sadasant added 3 commits September 2, 2021 00:30

feedback and tests fixed

63c2986

simpler changelog entry

8f93b29

MSICache is not necessary

e376252

sadasant requested a review from chlowell September 2, 2021 00:44

chlowell reviewed Sep 2, 2021

View reviewed changes

after feedback from Charles

d35daf5

sadasant commented Sep 2, 2021

View reviewed changes

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts Show resolved Hide resolved

sadasant added 3 commits September 2, 2021 20:21

Merge remote-tracking branch 'Azure/main' into identity/fix15800

18ee518

small improvements

f8deac2

using the original scopes instead of the altered resource, and better…

a19e628

… error messages

formatting

3df153d

sadasant added 2 commits September 2, 2021 23:32

possible CI fix

ad0f06f

will this fix the build?

67f1d04

sadasant commented Sep 3, 2021

View reviewed changes

sadasant and others added 9 commits September 3, 2021 17:28

one more CHANGELOG entry

2dd335c

overkill solution to ensure the environment is cleaned for this test.…

ab0c572

….. might fix CI

this time trying with a potential bug fix in the IMDS MSI

b87127f

i am running out of ideas

2881f6c

feedback after speaking with Scott Schaab: limit AZURE_CLIENT_ID to t…

9eb6203

…his new MSI, and improve MSI logs

Here we go again

385f187

another attempt

02fd239

somewhat desperate meassures: logging the error

0ec0125

ok found the source of the CI issue: one last non-standard error message

9b3811b

sadasant commented Sep 4, 2021

View reviewed changes

sadasant requested a review from chlowell September 7, 2021 18:58

chlowell reviewed Sep 7, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/models.ts Outdated Show resolved Hide resolved

small fix to mapScopesToResource after a second thought

2f07e3a

sadasant force-pushed the identity/fix15800 branch from bf2064e to 2f07e3a Compare September 7, 2021 22:07

chlowell reviewed Sep 7, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/utils.ts Outdated Show resolved Hide resolved

specifying that most MSIs are not available for getToken requests wit…

3091b74

…h multiple resources, and an internal cleanup was in order (including slightly better logs and errors

chlowell reviewed Sep 8, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts Outdated Show resolved Hide resolved

chlowell reviewed Sep 8, 2021

View reviewed changes

sadasant and others added 2 commits September 7, 2021 21:30

Update sdk/identity/identity/src/credentials/managedIdentityCredentia…

1dd2856

…l/tokenExchangeMsi.ts Co-authored-by: Charles Lowell <chlowe@microsoft.com>

testing that parameter client_id overrides environment client_id

a2a5f89

chlowell approved these changes Sep 8, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/tokenExchangeMsi.ts Outdated Show resolved Hide resolved

variable rename thanks to Charles

1cf0242

sadasant added the auto-merge label Sep 8, 2021

ghost merged commit 6ccd67c into Azure:main Sep 8, 2021

sadasant deleted the identity/fix15800 branch September 8, 2021 17:27

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Support exchanging k8s token to AAD token #16688

[Identity] Support exchanging k8s token to AAD token #16688

sadasant commented Jul 31, 2021

sadasant commented Sep 1, 2021

sadasant commented Sep 1, 2021

sadasant Sep 2, 2021

sadasant commented Sep 2, 2021

sadasant commented Sep 2, 2021

sadasant commented Sep 3, 2021

sadasant Sep 3, 2021

sadasant Sep 4, 2021

sadasant Sep 4, 2021

sadasant commented Sep 4, 2021

chlowell Sep 8, 2021

sadasant Sep 8, 2021

sadasant Sep 8, 2021

chlowell left a comment

ghost commented Sep 8, 2021

		@@ -4,6 +4,8 @@

		### Features Added

		- `ManagedIdentityCredential` now supports token exchange authentication.

[Identity] Support exchanging k8s token to AAD token #16688

[Identity] Support exchanging k8s token to AAD token #16688

Conversation

sadasant commented Jul 31, 2021

sadasant commented Sep 1, 2021

sadasant commented Sep 1, 2021

sadasant Sep 2, 2021

Choose a reason for hiding this comment

sadasant commented Sep 2, 2021

sadasant commented Sep 2, 2021

sadasant commented Sep 3, 2021

sadasant Sep 3, 2021

Choose a reason for hiding this comment

sadasant Sep 4, 2021

Choose a reason for hiding this comment

sadasant Sep 4, 2021

Choose a reason for hiding this comment

sadasant commented Sep 4, 2021

chlowell Sep 8, 2021

Choose a reason for hiding this comment

sadasant Sep 8, 2021

Choose a reason for hiding this comment

sadasant Sep 8, 2021

Choose a reason for hiding this comment

chlowell left a comment

Choose a reason for hiding this comment

ghost commented Sep 8, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.