-
Notifications
You must be signed in to change notification settings - Fork 3k
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 workaround for cross-tenant authentication with Track 2 SDKs #16797
Conversation
Core |
if external_tenant_tokens: | ||
# Hard-code scheme to 'Bearer' as _BearerTokenCredentialPolicyBase._update_headers does. | ||
client_kwargs['headers']['x-ms-authorization-auxiliary'] = \ | ||
', '.join("Bearer {}".format(t[1]) for t in external_tenant_tokens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can both ,
and ;
be used as the delimiter for each token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply don't know. We need to do some test.
The exiting code
aux_tokens = ';'.join(['{} {}'.format(scheme2, tokens2) for scheme2, tokens2, _ in external_tenant_tokens]) |
contradicts the document Authenticate requests across tenants
Bearer <auxiliary-token1>, EncryptedBearer <auxiliary-token2>, Bearer <auxiliary-token3>
That's why this PR is still marked as a draft. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my experiment of ARM deployment, the correct delimiter should be ,
.
When the delimiter is ;
, the service returns the following error:
{"error":{"code":"InvalidAuxiliaryTokens","message":"Authentication failed for auxiliary token: 'The auxiliary tokens should have proper format \"'scheme' 'value'\". The '1' auxiliary token(s) were not found in proper format. Invalid tokens are in the response header.'"}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! Also verified with:
> az rest --url "https://management.azure.com/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourcegroups?api-version=2020-10-01" --headers "x-ms-authorization-auxiliary=Bearer token1; Bearer token2" --verbose
Request URL: 'https://management.azure.com/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourcegroups?api-version=2020-10-01'
Request method: 'GET'
Request headers:
'Authorization': 'Bearer eyJ0eXAiOiJKV...'
'x-ms-authorization-auxiliary': 'Bearer token1; Bearer token2'
...
Response status: 401
Response headers:
'WWW-Authenticate': 'Bearer authorization_uri="https://login.windows.net/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a", error="invalid_token", error_description="Invalid auxiliary tokens \'Bearer token1; Bearer token2\'."'
...
Response content:
{"error":{"code":"InvalidAuxiliaryTokens","message":"Authentication failed for auxiliary token: 'The auxiliary tokens should have proper format \"'scheme' 'value'\". The '1' auxiliary token(s) were not found in proper format. Invalid tokens are in the response header.'"}}
Unauthorized({"error":{"code":"InvalidAuxiliaryTokens","message":"Authentication failed for auxiliary token: 'The auxiliary tokens should have proper format \"'scheme' 'value'\". The '1' auxiliary token(s) were not found in proper format. Invalid tokens are in the response header.'"}})
This means the doc is right and the current CLI code is wrong.
@jsntcy @msyyc, please kindly test for Network module and add some I have already written some testing script: #16691 (comment) You may refer to azure-cli/src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py Lines 4877 to 4913 in fa7939b
|
resource = cmd.cli_ctx.cloud.endpoints.active_directory_resource_id | ||
cred, _, _ = profile.get_login_credentials(resource=resource, | ||
aux_subscriptions=aux_subscriptions) | ||
_, _, _, external_tokens = cred.get_all_tokens('https://management.azure.com/.default') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By moving this to core, it now works for sovereign cloud (solves #15750 (comment)).
@@ -573,6 +574,10 @@ def get_login_credentials(self, resource=None, subscription_id=None, aux_subscri | |||
if sub[_TENANT_ID] != account[_TENANT_ID]: | |||
external_tenants_info.append(sub[_TENANT_ID]) | |||
|
|||
if external_tenants_info and (identity_type or in_cloud_console()): | |||
raise CLIError("Cross-tenant authentication is not supported by managed identity and Cloud Shell. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLIError is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is difficult to classify.
@@ -35,7 +35,7 @@ def _get_token(self, sdk_resource=None): | |||
""" | |||
external_tenant_tokens = None | |||
try: | |||
scheme, token, full_token = self._token_retriever(sdk_resource) | |||
scheme, token, token_entry = self._token_retriever(sdk_resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it just a rename operation? Does the object structure change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. No.
# As a temporary workaround, manually add external tokens to 'x-ms-authorization-auxiliary' header. | ||
# https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/authenticate-multi-tenant | ||
if getattr(cred, "_external_tenant_token_retriever", None): | ||
_, _, _, external_tenant_tokens = cred.get_all_tokens(*resource_to_scopes(resource)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is *
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_, _, _, external_tenant_tokens = cred.get_all_tokens(*resource_to_scopes(resource)) | ||
# Hard-code scheme to 'Bearer' as _BearerTokenCredentialPolicyBase._update_headers does. | ||
client_kwargs['headers']['x-ms-authorization-auxiliary'] = \ | ||
', '.join("Bearer {}".format(t[1]) for t in external_tenant_tokens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a pipeline support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make a new policy and add it to the pipeline in the future, utilizing Azure/azure-sdk-for-python#17340.
@@ -3363,8 +3345,7 @@ def create_image_version(cmd, resource_group_name, gallery_name, gallery_image_n | |||
gallery_name=gallery_name, | |||
gallery_image_name=gallery_image_name, | |||
gallery_image_version_name=gallery_image_version, | |||
gallery_image_version=image_version, | |||
headers={'x-ms-authorization-auxiliary': external_bearer_token} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You deleted lots of code in vm/custom.py and didn't add anything. How are external tokens handled now? Is another PR required to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You deleted lots of code in vm/custom.py and didn't add anything.
The logic is not deleted, but moved to _get_mgmt_service_client
.
How are external tokens handled now?
External tokens are handled automatically when aux_subscriptions
is provided to _get_mgmt_service_client
.
client = _compute_client_factory(cmd.cli_ctx, aux_subscriptions=aux_subscriptions)
Is another PR required to fix it?
It already works. No other PR needed.
Have you run live test for |
Co-authored-by: Feiyue Yu <iamyfy@163.com>
No. You may take some time to run it. |
Testing results of VM. Fail. Subscription is not found. Investigating. |
It is because the test relies on Microsoft tenant azure-cli/src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py Line 3846 in ba16edd
but your test account doesn't have permission there. |
Fix #16691:
az vnet peering create
no longer works cross-tenant⚠ This is only a temporary workaround for Azure/azure-sdk-for-python#8313.
Changes
Manually add external tokens to
x-ms-authorization-auxiliary
header.Regarding auto-refresh
As far as I know and also tested for asynchronous Azure operations (long-running operations)
x-ms-authorization-auxiliary
is only necessary in the first request:The response is a
200
withAzure-AsyncOperation
, which contradicts Track asynchronous Azure operations. (Azure/azure-rest-api-specs#12828)The following pooling request won't need
x-ms-authorization-auxiliary
and can also succeed:Therefore, we can believe tokens in
x-ms-authorization-auxiliary
is only used once, so auto-refresh within a long-running operation won't be necessary.Alternative solutions
Develop a custom policy
ExternalBearerTokenCredentialPolicy
and add it to the client. However, adding a custom policy to a client is current impracticable (Azure/azure-sdk-for-python#16519).