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

[Core] Add CAE flag to auth policies #31012

Merged
merged 9 commits into from
Jul 27, 2023
Merged

[Core] Add CAE flag to auth policies #31012

merged 9 commits into from
Jul 27, 2023

Conversation

pvaneck
Copy link
Member

@pvaneck pvaneck commented Jul 5, 2023

The get_token protocol is updated to allow an optional enable_cae keyword argument. The overall signature doesn't change as we just document that enable_cae can be passed in as a part of kwargs.

With the flag, we can enable users and client SDKs to speciify that get_token requests should be requesting CAE-enabled tokens.

If the underlying credential's get_token implementation supports this flag, then a CAE-enabled token should be requested. Otherwise, a non-CAE token should be requested.

In this PR BearerTokenCredentialPolicy and AsyncBearerTokenCredentialPolicy are updated to also allow an enable_cae keyword argument in the constructors. This will be used in determining if enable_cae should be used in their respective get_token requests.

Since, ARM supports CAE and has logic for handling these CAE claims challenges, ARMChallengeAuthenticationPolicy and AsyncARMChallengeAuthenticationPolicy were updated to ensure that enable_cae is set to True. Edit: This will be split out into a separate PR

More info here: #30777

Notes

  • From what I can tell, all current (non-test) get_token implementations across our SDKs take in **kwargs, so enable_cae being passed in shouldn't cause any breakage. If needed, we can always catch TypeErrors for unexpected keyword arguments.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core

pvaneck added 5 commits July 23, 2023 07:30
This enables users and client SDKs to pass in a flag to denote that
`get_token` requests should be requesting CAE tokens.

If the underlying credential's `get_token` implementation supports this
flag, then a CAE token will be requested. Otherwise, a non-CAE token
will be requested.

Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
pvaneck added 2 commits July 25, 2023 08:06
- This also adjusts test token credential `get_token` methods
  to accept kwargs if they do not.

Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
@pvaneck pvaneck merged commit a7519f9 into Azure:main Jul 27, 2023
@pvaneck pvaneck deleted the core-cae branch July 28, 2023 21:21
@david-becher
Copy link

When using az keyvault secret show we get an ERROR: Session.request() got an unexpected keyword argument 'enable_cae' error. Do not know, whether this is a problem of this library or of azure-cli, but only downgrading azure-core to 1.28.0 fixed the issue in the azure cli. @pvaneck @kashifkhan

vivekmore-msft pushed a commit that referenced this pull request Aug 9, 2023
This first adds a keyword argument to the TokenCredential protocol method `get_token`.

This enables users and client SDKs to pass in a flag to denote that
`get_token` requests should be requesting CAE tokens.

If the underlying credential's `get_token` implementation supports this
flag, then a CAE token will be requested. Otherwise, a non-CAE token
will be requested.

Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>

(cherry picked from commit a7519f9)
bharat-kalyan-namburi pushed a commit that referenced this pull request Aug 9, 2023
* Code refactored as per main

* Fixed issues

* Fixed testcases

* Reverted play_media_to_all changes

* Updated readme as per latest changes

* [Core] Add CAE flag to auth policies (#31012)

This first adds a keyword argument to the TokenCredential protocol method `get_token`.

This enables users and client SDKs to pass in a flag to denote that
`get_token` requests should be requesting CAE tokens.

If the underlying credential's `get_token` implementation supports this
flag, then a CAE token will be requested. Otherwise, a non-CAE token
will be requested.

Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>

(cherry picked from commit a7519f9)

* fix tests (#31526)

* fix tests

* update

(cherry picked from commit d1d0ef6)

* Packaging update of azure-mgmt-azureadb2c

---------

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
Co-authored-by: Xiang Yan <xiangsjtu@gmail.com>
Co-authored-by: Azure SDK Bot <adxpysdk@microsoft.com>
@xiangyan99
Copy link
Member

Thank you for reporting the issue.

Could you share the version of azure-cli you are using?

And if possible, the version of azure-keyvault package?

@david-becher
Copy link

@xiangyan99

The azure-cli was freshly installed via pipx in version 2.51.0. This installed dependency azure-core was in version 1.29.0 and azure-keyvault in version 1.1.0. As I said, manually downgrading azure-core to version 1.28.0 mitigated the problem.

@jiasli
Copy link
Member

jiasli commented Aug 10, 2023

From what I can tell, all current (non-test) get_token implementations across our SDKs take in **kwargs, so enable_cae being passed in shouldn't cause any breakage.

This assumption is unfortunately not true for Azure CLI and caused breakage in Azure CLI as shown in the above comment #31012 (comment) and Azure/azure-cli#27131.

Azure CLI also implements get_token protocol and passes **kwargs to MSAL almost untouched:

https://github.com/Azure/azure-cli/blob/21266e02457263420b62fba1c529f69e28852414/src/azure-cli-core/azure/cli/core/auth/msal_authentication.py#L69

    def get_token(self, *scopes, claims=None, **kwargs):
        ...
        result = self.acquire_token_silent_with_error(list(scopes), self._account, claims_challenge=claims, **kwargs)

The only exception is tenant_id which is popped from **kwargs.

https://github.com/Azure/azure-cli/blob/0fb75afa1e75f8a2419514c5fcf19533138f7185/src/azure-cli-core/azure/cli/core/auth/credential_adaptor.py#L62

    def get_token(self, *scopes, **kwargs):
        ...
        # SDK azure-keyvault-keys 4.5.0b5 passes tenant_id as kwargs, but we don't support tenant_id for now,
        # so discard it.
        kwargs.pop('tenant_id', None)

The popping tenant_id logic was added by Azure/azure-cli#21244 because of the breakage caused by passing tenant_id to get_token. See Azure/azure-cli#20880 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants