Support CAE #16852

msJinLei · 2022-01-17T05:55:46Z

Description

Implement CAE by adding handler to http pipeline
Improved error message when login is blocked by AAD
Improved error message when silent reauthentication failed
Enabled CAE for Get-AzTenant and Get-AzSubcription
Enable CAE for MSGraph
Added test cases

Related issue

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
  - {Please put the link here}
- the markdown help files have been regenerated using the commands listed here

msJinLei · 2022-01-23T12:23:10Z

/azp run azure-powershell - windows-powershell

azure-pipelines · 2022-01-23T12:23:20Z

Azure Pipelines successfully started running 1 pipeline(s).

msJinLei · 2022-01-23T12:23:34Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2022-01-23T12:23:44Z

Azure Pipelines successfully started running 1 pipeline(s).

wyunchi-ms · 2022-01-24T02:04:23Z

/azp run

azure-pipelines · 2022-01-24T02:04:43Z

Azure Pipelines successfully started running 3 pipeline(s).

isra-fel · 2022-01-24T05:24:10Z

Request @dolauli to review the part related to autorest-based cmdlets (src/Accounts/Accounts/CommonModule/ContextAdapter.cs)

isra-fel

One general comment: to avoid possible breaking change, we should expose as little APIs as we can. For example the ClaimsChallengerHandler class.

src/Accounts/Authenticators/MsalAccessToken.cs

src/Accounts/Authentication/Authentication/IClaimsChallengeProcessor.cs

src/Accounts/Authentication/Authentication/AuthenticationFailedExceptionExtention.cs

isra-fel · 2022-01-24T05:09:56Z

src/Accounts/Accounts/Account/ConnectAzureRmAccount.cs

+        private bool TryParseUnknownAuthenticationException(AuthenticationFailedException exception, out string message)
+        {
+


Shall we move the method out of this class because (1) this class is already very long (2) the logic of parsing MsalServiceException is not strongly tied to Connect-AzAccount (3) it's easier to test

We can have a static helper class that handles details of authentication exceptions.

Currently no other place to use the TryParseUnknownAuthenticationException and its logic is very specific for login. We can extract it when we find other place can share the logic.

isra-fel · 2022-01-24T05:12:53Z

src/Accounts/Accounts/ChangeLog.md

+* Implemented CAE by adding handler to http pipeline
+* Improved error message when login is blocked by AAD
+* Improved error message when silent reauthentication failed
+* Enabled CAE for Get-AzTenant and Get-AzSubcription
+* Enabled CAE for MSGraph
+* Added test cases


A few suggestions on the changelogs: (1) use the full term "continuous access evaluation" for the first appearance of "CAE" (2) hide implementation details that are not so useful to the customers (such as adding handler and test cases)

src/Accounts/Authentication/ClaimsChallengeHandler.cs

src/Accounts/Authenticators/MsalAccessToken.cs

isra-fel

LGTM. Have you tested the autorest-based modules manually?

isra-fel · 2022-01-27T09:30:04Z

src/Accounts/Accounts/CommonModule/ContextAdapter.cs

@@ -115,8 +117,7 @@ public void OnNewRequest(InvocationInfo invocationInfo, string correlationId, st
                {
                    endpointResourceIdKey = endpointResourceIdKey ?? AzureEnvironment.Endpoint.ResourceManager;
                    var context = GetDefaultContext(_provider, invocationInfo);
-                    await AuthorizeRequest(context, request, cancelToken, endpointResourceIdKey, endpointSuffixKey, tokenAudienceConverter);
-                    return await next(request, cancelToken, cancelAction, signal);


As we disscussed in today's meeting, we would like to revisit the part of the design related to autorest -- instead of adding CAE logic to the AddAuthorizeRequestHandler method, which has little to do with CAE, it seems better to spilt that into a new handler specially for CAE.

However, redoing authentication requires information about the access token (such as environment, audience). If the logic is departed from authentication, how does it get those info?

A second concern is about calling next(). We want to retry the failed request by calling next() for the second time (line 211), however if there are more handlers after this one, those handlers will be called twice against the same http request, which may have side-effect such as duplicated HTTP header. Is there a way allowing us to resend the request without duplicately calling the handlers?

As we disscussed in today's meeting, we would like to revisit the part of the design related to autorest -- instead of adding CAE logic to the AddAuthorizeRequestHandler method, which has little to do with CAE, it seems better to spilt that into a new handler specially for CAE.

However, redoing authentication requires information about the access token (such as environment, audience). If the logic is departed from authentication, how does it get those info?

A second concern is about calling next(). We want to retry the failed request by calling next() for the second time (line 211), however if there are more handlers after this one, those handlers will be called twice against the same http request, which may have side-effect such as duplicated HTTP header. Is there a way allowing us to resend the request without duplicately calling the handlers?

Thanks for @isra-fel's conclusions. After the discussion with @dolauli, we find a way to seperate the authentication and reauthentication steps. However, the codes of these 2 parts are coupled, we decide to include them into one step currently.
@dolauli Could you help to review the related codes?

Thanks

* prototype for CAE * CAE issue fix * Improved error message when login is blocked by AAD * Improved error message when silent reauthentication failed * Enable CAE for Get-AzTenant and Get-AzSubcription * Add test case Co-authored-by: Erich(Renyong) Wang <[email protected]>

…r autorest *Enable CAE for MSGraph (Azure#16766)

isra-fel · 2022-02-08T01:41:14Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2022-02-08T01:41:25Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Accounts/Accounts/CommonModule/ContextAdapter.cs

msJinLei · 2022-02-08T22:50:38Z

/azp run

azure-pipelines · 2022-02-08T22:50:58Z

Azure Pipelines successfully started running 3 pipeline(s).

dolauli

LGTM

isra-fel · 2022-02-09T08:35:02Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2022-02-09T08:35:12Z

Azure Pipelines successfully started running 1 pipeline(s).

isra-fel · 2022-02-10T02:47:40Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2022-02-10T02:47:50Z

Azure Pipelines successfully started running 1 pipeline(s).

msJinLei changed the title ~~Cae Draft~~ Support CAE Jan 21, 2022

msJinLei force-pushed the cae_2022 branch from 873c969 to e7cb1b0 Compare January 21, 2022 10:29

msJinLei marked this pull request as ready for review January 21, 2022 10:30

msJinLei requested review from wyunchi-ms, isra-fel and dingmeng-xue January 21, 2022 10:34

msJinLei added the needs-review label Jan 21, 2022

msJinLei force-pushed the cae_2022 branch from e7cb1b0 to a827b4f Compare January 23, 2022 13:31

msJinLei added this to the Jan 2022 (2022-02-08) milestone Jan 23, 2022

isra-fel requested changes Jan 24, 2022

View reviewed changes

msJinLei requested a review from isra-fel January 26, 2022 19:40

wyunchi-ms changed the base branch from main to release-2022-02-08 January 27, 2022 02:52

isra-fel approved these changes Jan 27, 2022

View reviewed changes

dingmeng-xue removed this from the Jan 2022 (2022-02-08) milestone Jan 27, 2022

isra-fel requested changes Jan 27, 2022

View reviewed changes

msJinLei and others added 5 commits February 7, 2022 18:09

Migrate on-claim-chanllenge handler to the new authentication step fo…

1c03b4d

…r autorest *Enable CAE for MSGraph (Azure#16766)

Update ChangeLog.md

6bf4340

Address review comments

ae63ed0

Fix duplicate request issue

4b524c2

msJinLei force-pushed the cae_2022 branch from ce57c95 to 4b524c2 Compare February 7, 2022 10:21

msJinLei requested review from bilaakpan-ms and grizzlytheodore as code owners February 7, 2022 10:21

msJinLei requested review from haagha and Sandido as code owners February 7, 2022 10:21

msJinLei changed the base branch from release-2022-02-08 to main February 7, 2022 10:22

msJinLei requested a review from isra-fel February 7, 2022 10:40

isra-fel removed request for grizzlytheodore, Sandido, bilaakpan-ms and haagha February 8, 2022 01:43

isra-fel self-assigned this Feb 8, 2022

dolauli reviewed Feb 8, 2022

View reviewed changes

src/Accounts/Accounts/CommonModule/ContextAdapter.cs Outdated Show resolved Hide resolved

Address review comments

6b023d1

msJinLei requested a review from dolauli February 9, 2022 00:14

dolauli reviewed Feb 9, 2022

View reviewed changes

dolauli approved these changes Feb 9, 2022

View reviewed changes

isra-fel approved these changes Feb 9, 2022

View reviewed changes

msJinLei merged commit 6518976 into Azure:main Feb 10, 2022

		private bool TryParseUnknownAuthenticationException(AuthenticationFailedException exception, out string message)
		{

Support CAE #16852

Support CAE #16852

Uh oh!

Conversation

msJinLei commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue

Checklist

Uh oh!

msJinLei commented Jan 23, 2022

Uh oh!

azure-pipelines bot commented Jan 23, 2022

Uh oh!

msJinLei commented Jan 23, 2022

Uh oh!

azure-pipelines bot commented Jan 23, 2022

Uh oh!

wyunchi-ms commented Jan 24, 2022

Uh oh!

azure-pipelines bot commented Jan 24, 2022

Uh oh!

isra-fel commented Jan 24, 2022

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

msJinLei Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

isra-fel Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

isra-fel Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

msJinLei Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Feb 8, 2022

Uh oh!

azure-pipelines bot commented Feb 8, 2022

Uh oh!

Uh oh!

msJinLei commented Feb 8, 2022

Uh oh!

azure-pipelines bot commented Feb 8, 2022

Uh oh!

dolauli left a comment

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Feb 9, 2022

Uh oh!

azure-pipelines bot commented Feb 9, 2022

Uh oh!

isra-fel commented Feb 10, 2022

Uh oh!

azure-pipelines bot commented Feb 10, 2022

Uh oh!

Uh oh!

msJinLei commented Jan 17, 2022 •

edited

Loading