[KeyVault] Challenge based authentication parallel fix, including tests #9059

sadasant · 2020-05-21T10:14:55Z

The challenge based authentication hotfix introduces a bug in which: Either when the client is not authenticated, or when the token is invalid, parallel network requests end up in a race condition where while one request is authenticating, the other ones fail immediately. This is not a problem before the hotfix because the challenge based authentication was always re-authenticating before making a request.

The parallel problem in more detail is as follows:

A new token was generated only if response.status == 401, and we received a www_authenticate header, which is ok, but parallel requests would choke at the !challenge.equalTo(this.challengeCache.challenge) mark, since by the time the second request compared the challenges, they would be the same, but the "response" set at the end was the 401 one, and not a new request made with the valid challenge. The fix consists of re-loading the credentials, and re-sending the request if the challenge happens to be valid at that point.

TODOs:

DONE: ~~Do the same for Secrets and Certificates (should I do this in separate PRs?).~~
Do this same process in the hotfix branches for KeyVault.

~~Keep in mind that this PR is not about refactoring the entirety of the challenge based authentication, which I'll be doing separately, and I'm tracking it with this issue: #9058~~

Update: Decided to refactor the challenge auth here, to make it easier to test it. Therefore:

Fixes #9058

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

heaths · 2020-05-21T18:00:41Z

sdk/keyvault/keyvault-keys/.prettierignore

Why the root slash? Does that still always resolve to the root? The old line didn't have it.

Without the root hash, I can't re-include the challengeBasedAuth file that is in the code folder :/ I will clean this up once we move this to a common folder.

huh, why are we ignoring these files anyway?

Here's the common folder PR that I'm proposing: #8866

@xirzec the files in the src/core folder should only come from the swagger tool we use, which makes it so that any changes we make are futile :/ ... except for the challenge based authentication code! which we should move out of there.

ohh... can we rename the folder to 'generated' at some point to make this clear?

Here's an issue: #9069

heaths · 2020-05-21T18:05:18Z

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

 import { Constants } from "@azure/core-http";
 import { HttpOperationResponse } from "@azure/core-http";
 import { HttpHeaders } from "@azure/core-http";
 import { WebResource } from "@azure/core-http";
 import { AccessTokenCache, ExpiringAccessTokenCache } from "@azure/core-http";

+type ValidParsedWWWAuthenticateProperties =
+  | "authorization"
+  | "authorization_url"


In .NET, we have this as "authorization_uri". Are you sure it's "_url"? Adding @schaabs who wrote that originally.

I'm sure that's my mistake. I'll move to authorization_uri. Thank you!

If you're sure. I'm not so sure myself, which is why I included Scott. I'm not sure how to verify it as well. :)

The authorization_uri was most likely copied from the track 1 code. If we're not sure probably best to default to what was in .NET track 1 since that SDK predates the rest. Either way this might just be a moot point left over from an older API version we're not supporting. Looking at the test recordings 7.0 seems to pass back "autorization" consistently in the challenge

"WWW-Authenticate": "Bearer authorization=\u0022https:\u002f\u002flogin.windows.net\*****, resource=\u0022https:\u002f\u002fvault.azure.net\u0022",

Sounds good!

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

heaths · 2020-05-21T18:09:57Z

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

+    // or if the cached challenge has a different scope,
+    // we store the just received challenge and reset the cached token, to force a re-authentication.
+    // This is exactly what C# is doing, as we can see here:
+    // https://github.com/heaths/azure-sdk-for-net/blob/a35434c1ce955d4cdeb393748f9f6407d4a984e2/sdk/keyvault/Azure.Security.KeyVault.Shared/src/ChallengeBasedAuthenticationPolicy.cs#L233


I appreciate the nod, but might want to link to the main repo's source? 😉

I mean, I would get lost in the main repo's source! I want to link to what's similar in that code. Let me know if you know a better link.

Maybe what you want is this?

https://github.com/Azure/azure-sdk-for-net/blob/70e54b878ff1d01a45266fb3674a396b4ab9c1d2/sdk/keyvault/Azure.Security.KeyVault.Shared/src/ChallengeBasedAuthenticationPolicy.cs#L143-L147

Wait, so you do compare the authority? hmm

Ok! Updated!

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

sdk/keyvault/keyvault-keys/test/challengeBasedAuthenticationPolicy.test.ts

xirzec

Small suggestion

xirzec · 2020-05-21T19:28:41Z

sdk/keyvault/keyvault-keys/.prettierignore

huh, why are we ignoring these files anyway?

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts

ramya-rao-a · 2020-05-26T05:37:04Z

Do the same for Secrets and Certificates (should I do this in separate PRs?).
Do this same process in the hotfix branches for KeyVault.

I would recommend changing secrets and certificates in the same PR for the master branch.

For hotfix, just like last time, we can use 1 PR for keys & secrets, another for certificates.

sadasant · 2020-05-26T12:04:36Z

As a note on the wording, I'm using "parallel" since these promises are being executed at the same time, at least from the perspective of the developer. I'm using this as reference: https://takuti.me/note/parallel-vs-concurrent/

Let me know if I should change it.

sadasant · 2020-05-26T13:52:00Z

sdk/keyvault/keyvault-certificates/.eslintignore

@@ -0,0 +1,2 @@
+# Ignoring the core files since they're auto-generated. Eventually, the auto-generated code will be on par with our current eslint rules, but in the mean time we should ignore them.
+src/core


This file was missing in certificates.

ramya-rao-a · 2020-05-26T15:50:04Z

@jonathandturner, @xirzec, Can you take another look now that the PR has been update to work with secrets and certificates as well?

@sadasant, Please update the PR title and have the hotfix PRs created as well

xirzec

I think this will work as is, but left a couple suggestions for further polish.

sdk/keyvault/keyvault-certificates/package.json

sdk/keyvault/keyvault-certificates/src/core/challengeBasedAuthenticationPolicy.ts

xirzec · 2020-05-27T00:09:08Z

sdk/keyvault/keyvault-certificates/src/core/challengeBasedAuthenticationPolicy.ts

+    // - An authorization URI with a token,
+    // - The resource to which that token is valid against (also called the scope).
+    const parsedWWWAuth = this.parseWWWAuthenticate(wwwAuthenticate);
+    const authorization = parsedWWWAuth.authorization!;


feels weird to do all these ! operations to assert something exists. If we know it exists, why make it optional on the type? If we don't know that it exists, why not add some checks so we don't blow up at runtime?

I'll add an exception case if we don't receive the values that we expect! Thank you!

I added this:

if (!(authorization && resource)) { return this._nextPolicy.sendRequest(webResource); }

sdk/keyvault/keyvault-certificates/src/core/challengeBasedAuthenticationPolicy.ts

xirzec · 2020-05-27T00:14:38Z

sdk/keyvault/keyvault-certificates/src/core/challengeBasedAuthenticationPolicy.ts

-      }
-      return response;
-    } else {
+    // If we don't receive a response with a 401 status code,


can everything from here until the end be moved up after the line webResource.body = originalBody inside the body of the if? Since we didn't blank the body in the else, we know it will never respond with 401 and we can just return the response.

Do we know it will never return 401? Seems like we are being extra cautious if we double check like this.

Let's say we're already authenticated, but the server answers with 401 when our code tries to:

await this.loadToken(webResource); response = await this._nextPolicy.sendRequest(webResource);

Seems possible! I'm assuming that if the challenge or token have some kind of expiration, this might help us correctly re-authenticate.

(I'm seeing an A-zure pun and I don't know what to do with it 😄)

- Adds changes missing from #9059

Challenge based auth fix, including a test

53b7157

sadasant added Client This issue points to a problem in the data-plane of the library. KeyVault labels May 21, 2020

sadasant requested review from sophiajt and xirzec May 21, 2020 10:14

sadasant self-assigned this May 21, 2020

sadasant mentioned this pull request May 21, 2020

[KeyVault] Challenge based auth feedback #9039

Closed

the recordings

913248e

sadasant mentioned this pull request May 21, 2020

Keyvault-Secrets: Request is missing a Bearer or PoP token error after upgrading to 4.0.3 #9005

Closed

6 tasks

sadasant commented May 21, 2020

View reviewed changes

sdk/keyvault/keyvault-keys/src/core/challengeBasedAuthenticationPolicy.ts Outdated Show resolved Hide resolved

better tests, and refactored the challenge based auth

9d4c8c6

sadasant requested a review from ramya-rao-a May 21, 2020 13:59

sadasant marked this pull request as ready for review May 21, 2020 13:59

sadasant changed the title ~~Challenge based auth fix, including a test~~ [KeyVault-Keys] Challenge based auth fix, including a test May 21, 2020

ramya-rao-a requested a review from daviwil May 21, 2020 16:18

heaths approved these changes May 21, 2020

View reviewed changes

xirzec reviewed May 21, 2020

View reviewed changes

feedback so far

c8eb7e3

heaths approved these changes May 21, 2020

View reviewed changes

sadasant added 3 commits May 21, 2020 21:33

Scott feedback Azure#9059 (comment)

287b247

Better link from Heath Azure#9059 (comment)

8f13098

formatting

aca8600

sophiajt approved these changes May 24, 2020

View reviewed changes

sadasant changed the title ~~[KeyVault-Keys] Challenge based auth fix, including a test~~ [KeyVault-Keys] Challenge based authentication concurrency fix, including tests May 26, 2020

sadasant changed the title ~~[KeyVault-Keys] Challenge based authentication concurrency fix, including tests~~ [KeyVault-Keys] Challenge based authentication parallel fix, including tests May 26, 2020

sadasant added 3 commits May 26, 2020 12:20

changes on the other clients, minus recordings

adbbf23

recordings and other minor tweaks

48ef2e9

recordings

f89ca0c

sadasant added 2 commits May 26, 2020 13:34

Merge remote-tracking branch 'Azure/master' into fix9005

bf1d4a4

updated sinon

48db09d

sadasant commented May 26, 2020

View reviewed changes

ramya-rao-a requested a review from sophiajt May 26, 2020 15:48

sadasant changed the title ~~[KeyVault-Keys] Challenge based authentication parallel fix, including tests~~ [KeyVault] Challenge based authentication parallel fix, including tests May 26, 2020

sadasant mentioned this pull request May 26, 2020

[KeyVault Keys] [KeyVault Secrets] v4.0.4 Challenge based authentication hotfix #9120

Merged

xirzec approved these changes May 27, 2020

View reviewed changes

sadasant mentioned this pull request May 27, 2020

[KeyVault Certificates] v4.0.2 hotfix for #9005 and #9020 #9122

Merged

sadasant and others added 3 commits May 27, 2020 00:45

alphabetically inserting sinon on the package.jsons

0a776b9

feedback by Jeff

12b14c4

Merge branch 'master' into fix9005

c923099

sophiajt approved these changes May 27, 2020

View reviewed changes

Merge branch 'master' into fix9005

f07e346

sadasant merged commit af3f6b3 into Azure:master May 27, 2020

sadasant deleted the fix9005 branch May 27, 2020 21:55

mikeharder added a commit that referenced this pull request May 29, 2020

rush update

f52d803

- Adds changes missing from #9059

mikeharder mentioned this pull request May 29, 2020

rush update #9170

Merged

mikeharder added a commit that referenced this pull request May 29, 2020

rush update (#9170)

f42039b

- Adds changes missing from #9059

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KeyVault] Challenge based authentication parallel fix, including tests #9059

[KeyVault] Challenge based authentication parallel fix, including tests #9059

sadasant commented May 21, 2020 •

edited

Loading

heaths May 21, 2020

sadasant May 21, 2020

xirzec May 21, 2020

sadasant May 21, 2020

sadasant May 21, 2020 •

edited

Loading

xirzec May 21, 2020

sadasant May 21, 2020

heaths May 21, 2020

sadasant May 21, 2020

heaths May 21, 2020

schaabs May 21, 2020 •

edited

Loading

sadasant May 21, 2020

heaths May 21, 2020

sadasant May 21, 2020

heaths May 21, 2020

sadasant May 21, 2020

sadasant May 21, 2020

xirzec left a comment

xirzec May 21, 2020

ramya-rao-a commented May 26, 2020

sadasant commented May 26, 2020

sadasant May 26, 2020

ramya-rao-a commented May 26, 2020

xirzec left a comment

xirzec May 27, 2020

sadasant May 27, 2020

sadasant May 27, 2020

xirzec May 27, 2020

sadasant May 27, 2020

sadasant May 27, 2020 •

edited

Loading

xirzec May 27, 2020

sadasant May 27, 2020

		@@ -0,0 +1,2 @@
		# Ignoring the core files since they're auto-generated. Eventually, the auto-generated code will be on par with our current eslint rules, but in the mean time we should ignore them.
		src/core

[KeyVault] Challenge based authentication parallel fix, including tests #9059

[KeyVault] Challenge based authentication parallel fix, including tests #9059

Conversation

sadasant commented May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schaabs May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented May 26, 2020

sadasant commented May 26, 2020

Choose a reason for hiding this comment

ramya-rao-a commented May 26, 2020

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented May 21, 2020 •

edited

Loading

sadasant May 21, 2020 •

edited

Loading

schaabs May 21, 2020 •

edited

Loading

sadasant May 27, 2020 •

edited

Loading