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

Continuous Access Evaluation (CAE) doesn't seem to be working for device code flow #21978

Closed
baywet opened this issue May 24, 2022 · 17 comments
Closed
Assignees
Labels
Azure.Core Azure.Identity Client This issue points to a problem in the data-plane of the library. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@baywet
Copy link
Member

baywet commented May 24, 2022

Hi from the Microsoft Graph SDKs team 👋

We're in the process of implementing CAE in our SDKs, which rely on Azure identity.

First, the GetTokenOptions interface doesn't expose a "claims" property, when the dotnet equivalent does as well as the Java equivalent. This is a limitation for us as we're only taking a dependency on core-auth in an effort to release only as often as needed and to give more control to customers over which azure-identity version they want to use. It's only defined in CredentialFlowGetTokenOptions which as far as I understand is only meant to be used by azure identity to interface with MSAL.

Now, if we force set the claim by doing something like (options as any).claims = claimsValue; the client with a device code flow still doesn't get a new challenge prompt when the session is revoked.

As per the claims value, from reading the code I understood the claims value expects the JSON representation (base64 decode the claim value from the response header, but do not JSON parse it).

Here are my questions:

  • Can you confirm CAE with device code flow works with this JS lib? (it does for Java and dotnet)
  • Can you confirm we're passing the right value? (base64 decoded, but not JSON parsed)
  • Could you move the claims property to the GetTokenOptions interface?
  • Could you validate the following repro is correctly configured?
const getClaimsFromResponse = (response: Response ) => {
  if (response.status === 401) {
    const rawAuthenticateHeader = response.headers.get("WWW-Authenticate");
    if (rawAuthenticateHeader && /^Bearer /gi.test(rawAuthenticateHeader)) {
	const rawParameters = rawAuthenticateHeader.replace(/^Bearer /gi, "").split(",");
	for (const rawParameter of rawParameters) {
		const trimmedParameter = rawParameter.trim();
		if (/claims="[^"]+"/gi.test(trimmedParameter)) {
			return trimmedParameter.replace(/claims="([^"]+)"/gi, "$1");
		}
	}
     }
   }
  return undefined;
};
const tokenCredentials = new DeviceCodeCredential(
{
  tenantId: '<tid>',
  clientId: '<cid>',
  userPromptCallback: (info) => console.log(info.message),
});
let previousClaims: string | undefined = undefined;
while(true) {
      const token = await tokenCredentials.getToken(["User.Read"], {claims: (previousClaims ? Buffer.from(previousClaims, "base64").toString(): undefined)} as any as GetTokenOptions)
      const previousResponse = await fetch("https://graph.microsoft.com/v1.0/me", {headers: {Authorization: `Bearer ${token.token}`}});
      previousClaims = getClaimsFromResponse(previousResponse);
      console.log("result", await previousResponse.text());
      await new Promise(resolve => setTimeout(resolve, 10000));
    }
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label May 24, 2022
@azure-sdk
Copy link
Collaborator

Label prediction was below confidence level 0.6 for Model:ServiceLabels: 'Azure.Core:0.47996175,Azure.Identity:0.2467415,IoT:0.03852034'

@jeremymeng
Copy link
Member

The identity doc seems to suggest to base64-decode the claims from the response, url-encode it then add it to the claims parameter of the authorization request

@jeremymeng
Copy link
Member

I think it makes sense to have claims on GetTokenOptions for consistency with other languages. @KarishmaGhiya @mpodwysocki

@baywet
Copy link
Member Author

baywet commented May 25, 2022

Thanks for the reply @jeremymeng After adding encodeURIComponent (after the base64 decode), this is still not working as expected.
I also added setLogLevel("info"); to get a better understanding of what's happening, and it seems the library is ignoring the claims property all together. Here are the logs

claims: eyJhY2Nlc3NfdG9rZW4iOnsibmCHANGEDAFEWCCHARACTERS2VudGlhbCI6dHJ1ZSwgInZhbHVlIjoiMTY1MzQ4NTQ0OSJ9fX0=
azure:identity:info DeviceCodeCredential => Attempting to acquire token silently
azure:identity:info DeviceCodeCredential => getToken() => SUCCESS. Scopes: User.Read.
result {"error":{"code":"InvalidAuthenticationToken","message":"Continuous access evaluation resulted in claims challenge with result: InteractionRequired and code: TokenIssuedBeforeRevocationTimestamp","innerError":{"date":"2022-05-25T13:30:59","request-id":"b61f7ba2-8ded-4666-8a55-aaf3ee9cd577","client-request-id":"b61f7ba2-8ded-4666-8a55-aaf3ee9cd577"}}}

I'd expect Azure Identity to either fail on the silent token acquisition and fallback on a full acquisition using the claims, or for it to "see the claims" and start a full acquisition from the get go.

@baywet

This comment was marked as outdated.

@baywet

This comment was marked as outdated.

@baywet
Copy link
Member Author

baywet commented May 25, 2022

after some more investigation I can confirm that the claims goes all the way here, but msal replies with the same access token that it already returned, from the cache.

this.logger.info("Attempting to acquire token silently");

@baywet
Copy link
Member Author

baywet commented May 25, 2022

After more investigation, azure identity 2.0.4 (current version at the time of writing) uses msal node 1.3.0

MSAL node 1.7.0 has a bugfix specific to claims and tokens caching.

For sanity, the dependency should be updated on the next patch/minor.

Also the claims need to be base64 decoded but not URI encoded, MSAL does the latter part for you.

@xirzec
Copy link
Member

xirzec commented May 25, 2022

After more investigation, azure identity 2.0.4 (current version at the time of writing) uses msal node 1.3.0

Since we depend on ^1.3.0, which is compatible with 1.8.0, we actually do use 1.8.0 currently:

'@azure/msal-node': 1.8.0

We could still bump up the minimum version to be higher in order to force folks to get the bugfix, though. @KarishmaGhiya can you take care of this?

I'm a bit confused about the scenario here, since I believe we handle CAE challenges in a pipeline policy rather than in the token credentials individually: https://github.com/Azure/azure-sdk-for-js/blob/c76126f3dac544df9c2801b67cbdae64aa18b647/sdk/core/core-client/src/authorizeRequestOnClaimChallenge.ts

@xirzec xirzec added question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Client This issue points to a problem in the data-plane of the library. Azure.Identity Azure.Core labels May 25, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label May 25, 2022
@baywet
Copy link
Member Author

baywet commented May 26, 2022

The Microsoft graph sdks are not using the same http pipeline azure sdks are. They are however using the same authentification layer.

@xirzec
Copy link
Member

xirzec commented May 26, 2022

The Microsoft graph sdks are not using the same http pipeline azure sdks are.

Would you be willing to try it out? You can configure your own pipeline with all custom policies or pick and choose which of our policies make sense for you.

@baywet
Copy link
Member Author

baywet commented May 26, 2022

That original decision predates my time on the team. I believe @darrelmiller had conversations with your architects at the time.

@KarishmaGhiya
Copy link
Member

@baywet We will be releasing a new GA 2.1.0 for Identity within the next two weeks, which will have dependency on the latest version of MSAL node. Would that work for you or would you prefer a bug fix for 2.0.x version instead?

@baywet
Copy link
Member Author

baywet commented Jun 1, 2022

@KarishmaGhiya thanks for the follow up. This would be enough for us, no need for an intermediate release.

@KarishmaGhiya
Copy link
Member

@baywet Heads up! Our GA 2.1.0 has been delayed to July. Let me know if you have any questions/ concerns.

@KarishmaGhiya
Copy link
Member

KarishmaGhiya commented Jul 12, 2022

@baywet We have released our GA 2.1.0 - https://www.npmjs.com/package/@azure/identity

Let me know if this solves your issue or if you have any questions/ concerns.

@baywet
Copy link
Member Author

baywet commented Jul 12, 2022

Thanks for the follow up. Nothing else required from our side. Closing

@baywet baywet closed this as completed Jul 12, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this issue Feb 1, 2023
Machinelearningservices microsoft.machine learning services 2022 12 01 preview (Azure#21761)

* Adds base for updating Microsoft.MachineLearningServices from version preview/2022-10-01-preview to version 2022-12-01-preview

* Updates readme

* Updates API version in new specs and examples

* Add Dec API Registries Swagger (Azure#21419)

* add december registries swagger + examples

* add status code 202 in examples

* fix 202 examples

* fixes

* fixes

* fix

* add 202 back in for put/patch

Co-authored-by: Komal Yadav <komalyadav@microsoft.com>

* remove location (Azure#21430)

Co-authored-by: Komal Yadav <komalyadav@microsoft.com>

* remove readonly flag on schedules property for CI (Azure#21653)

Co-authored-by: Naman Agarwal <naagarw@microsoft.com>

* add missing workspace properties (Azure#21725)

* December preview updating mfe.json specs (Azure#21510)

* December preview updating mfe.json specs

* MFE Dec 2022 Preview API - Adding logbase

* MFE 2022-12-01-preview swagger spec model validation fix

* MFE 2022-12-01-preview swagger spec model validation fix, add missing location

* MFE 2022-12-01-preview swagger spec model validation - typo fix

* MFE 2022-12-01-preview swagger spec model validation - fix api version in automljob example

* MFE 2022-12-01-preview swagger spec model validation - fix for multiselectenabled error

* MFE 2022-12-01-preview swagger spec model validation - fix for multiselectenabled error

* Fix  for 1006 - RemovedDefinition (RecurrenceTrigger,CronTrigger) (Azure#21822)

* fix ReadonlyPropertyChanged of MLC (Azure#21814)

Co-authored-by: Bingchen Li <bingchenli@microsoft.com>

* fixed custom-words conflict (Azure#21829)

* fix custom-words conflict merge (Azure#21830)

* example fix (INVALID_REQUEST_PARAMETER) (Azure#21832)

Co-authored-by: Ivaliy Ivanov <ivaliyivanov@Ivaliys-MacBook-Air.local>

* example fix, use correct api preview version  - (INVALID_REQUEST_PARAMETER) (Azure#21833)

Co-authored-by: Ivaliy Ivanov <ivaliyivanov@Ivaliys-MacBook-Air.local>

* Revert breaking change for MLC swagger 2022-12-01-preview (Azure#21885)

Co-authored-by: Bingchen Li <bingchenli@microsoft.com>

* Revert Connection Category back to enum. (Azure#21939)

* revert provisioning state change (Azure#21940)

* remove body (Azure#21978)

Co-authored-by: Komal Yadav <komalyadav@microsoft.com>

* Addressed comments, added x-ms-long-running-operation to a patch call (Azure#22005)

* Addressed comments, added x-ms-long-running-operation to a patch call

* fix examples for patch - remove body

* fixed formatting

* Ivalbert fix patch2 (Azure#22006)

* Addressed comments, added x-ms-long-running-operation to a patch call

* fix examples for patch - remove body

* fixed formatting

* fixed formatting

* Updated custom words (Azure#22262)

* Fixed prettier errors (Azure#22237)

* fixed examples for LRO_RESPONSE_HEADER check (Azure#22293)

* fixed examples for LRO_RESPONSE_HEADER check (Azure#22294)

* Example fix - OBJECT_MISSING_REQUIRED_PROPERTY - Missing required property: triggerType (#22317)

---------

Co-authored-by: Komal Yadav <23komal.yadav23@gmail.com>
Co-authored-by: Komal Yadav <komalyadav@microsoft.com>
Co-authored-by: Naman Agarwal <namanag16@gmail.com>
Co-authored-by: Naman Agarwal <naagarw@microsoft.com>
Co-authored-by: ZhidaLiu <zhili@microsoft.com>
Co-authored-by: libc16 <88697960+libc16@users.noreply.github.com>
Co-authored-by: Bingchen Li <bingchenli@microsoft.com>
Co-authored-by: Ivaliy Ivanov <ivaliyivanov@Ivaliys-MacBook-Air.local>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Azure.Identity Client This issue points to a problem in the data-plane of the library. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

5 participants