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

{Profile} az account get-access-token: Show expiresOn for managed identity #20219

Merged
merged 10 commits into from
Nov 17, 2021

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Nov 5, 2021

Fix #20211
Fix getporter/azure-plugins#39
Fix microsoft/AzureTRE#1067
Fix Azure/azure-cli-extensions#4076

Description

Issue: During the migration to MSAL, for managed identity, the expiresOn property in az account get-access-token's output is lost.

$ az account get-access-token
{
  "accessToken": "...",
  "expiresOn": null,
  "subscription": "0b1f6471-1bf0-4dda-aec3-cb9272f09590",
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a",
  "tokenType": "Bearer"
}

This is due to the complexity and inconsistency of expiresOn/expires_on properties across different services and tools.

There are 4 forms of expiresOn/expires_on:

  1. epoch str (VM managed identity endpoint 2018-02-01): "expires_on": "1605238724"
  2. epoch int (Azure Python SDK Track 2): "expires_on": 1605238724
  3. datetime in ISO str representing local time (ADAL token entry): "expiresOn": "2020-11-12 13:50:47.114324"
  4. datetime in Month/Day/Year str with timezone (App service managed identity endpoint 2017-09-01): "expires_on": "11/05/2021 15:18:31 +00:00"

This PR unified these 4 types:

get_raw_token returns

Testing Guide

apt install git
git clone https://github.com/jiasli/azure-cli --branch expires-on --depth 1
apt install python3-venv --yes
python3 -m venv py
. py/bin/activate
pip install -U pip
pip install azdev
azdev setup -c
az login --identity --debug
az account get-access-token --debug

@jiasli jiasli self-assigned this Nov 5, 2021
@yonzhan yonzhan added this to the Nov 2021 (2021-12-07) milestone Nov 5, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Nov 5, 2021

Profile

@@ -78,7 +78,7 @@ def get_access_token(cmd, subscription=None, resource=None, scopes=None, resourc
'tokenType': creds[0],
'accessToken': creds[1],
# 'expires_on': creds[2].get('expires_on', None),
'expiresOn': creds[2].get('expiresOn', None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to change this line?

Copy link
Member Author

@jiasli jiasli Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to guarantee that expiresOn always exists in creds[2] - CLI should fail if expiresOn is not set, instead of returning None which will cause more trouble.

from .auth.util import scopes_to_resource
msi_creds = MsiAccountTypes.msi_auth_factory(identity_type, identity_id,
scopes_to_resource(scopes))
sdk_token = msi_creds.get_token(*scopes)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old implementation of ADAL-based Azure CLI returns token_entry containing inconsistent fields:

  • expires_on for managed identity
  • expiresOn for ADAL credential

They are unified later at command module level:

if 'expires_on' in token_entry:
# https://docs.python.org/3.8/library/datetime.html#strftime-and-strptime-format-codes
token_entry['expiresOn'] = _fromtimestamp(int(token_entry['expires_on']))\
.strftime("%Y-%m-%d %H:%M:%S.%f")

We use get_token to unify them to epoch int expires_on in core instead.

Copy link
Contributor

@zhoxing-ms zhoxing-ms Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, will token_entry['expiresOn'] has its own value in some cases before it is overwritten by _fromtimestamp(int(token_entry['expires_on'])).strftime("%Y-%m-%d %H:%M:%S.%f")?
May I ask their values should be the same in all cases, but only in different formats, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. get_token only returns AccessToken which must have int epoch expires_on.

@waylew-lexis
Copy link

any update on when this issue will be merged and released? getting similar issue when trying to use azure app service with custom container.
ERROR: The command failed with an unexpected error. Here is the traceback:
ERROR: invalid literal for int() with base 10: '11/16/2021 14:26:35 +00:00'
Traceback (most recent call last):
File "/usr/lib/python3.8/site-packages/knack/cli.py", line 231, in invoke
cmd_result = self.invocation.execute(args)
File "/usr/lib/python3.8/site-packages/azure/cli/core/commands/init.py", line 657, in execute
raise ex
File "/usr/lib/python3.8/site-packages/azure/cli/core/commands/init.py", line 720, in _run_jobs_serially
results.append(self._run_job(expanded_arg, cmd_copy))
File "/usr/lib/python3.8/site-packages/azure/cli/core/commands/init.py", line 691, in _run_job
result = cmd_copy(params)
File "/usr/lib/python3.8/site-packages/azure/cli/core/commands/init.py", line 328, in call
return self.handler(*args, **kwargs)
File "/usr/lib/python3.8/site-packages/azure/cli/core/commands/command_operation.py", line 121, in handler
return op(**command_args)
File "/usr/lib/python3.8/site-packages/azure/cli/command_modules/profile/custom.py", line 84, in get_access_token
token_entry['expiresOn'] = _fromtimestamp(int(token_entry['expires_on']))
ValueError: invalid literal for int() with base 10: '11/16/2021 14:26:35 +00:00'
To open an issue, please run: 'az feedback'

@jiasli jiasli changed the title {Profile} az account get-access-token show expiresOn for managed identity {Profile} az account get-access-token: Show expiresOn for managed identity Nov 16, 2021
@jiasli
Copy link
Member Author

jiasli commented Nov 16, 2021

@waylew-lexis your issue has been resolved by #20215 and will be released in Azure CLI 2.31.0.

# Conflicts:
#	src/azure-cli-core/azure/cli/core/tests/test_profile.py
@@ -63,7 +61,6 @@ def __init__(self, *args, **kwargs):
self.object_id = kwargs.get('object_id')
self.msi_res_id = kwargs.get('msi_res_id')
self.resource = kwargs.get('resource')
MSRestAzureAuthStub.return_value = self
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't save the instance as class attribute so that tests won't interfere with each other.

@jiasli jiasli requested a review from evelyn-ys November 17, 2021 03:11
msi_creds.set_token()
token_entry = msi_creds.token
creds = (token_entry['token_type'], token_entry['access_token'], token_entry)
raise CLIError("Tenant shouldn't be specified for managed identity account")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use a specific error type (such as ArgumentUsageError) instead of CLIError?

Copy link
Member Author

@jiasli jiasli Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are so many places in core that don't comply with the error handling rule. Let's refine them together later.

@jiasli jiasli merged commit d7e5ede into Azure:dev Nov 17, 2021
@jiasli jiasli deleted the expires-on branch November 17, 2021 08:33
@Maarc-D
Copy link

Maarc-D commented Nov 18, 2021

Hello,

do you know when this merged modification will be integrated to a release ?
Because I have a function app that faild because of :

File "/home/site/wwwroot/.python_packages/lib/site-packages/azure/cli/core/adal_authentication.py", line 168, in get_token return AccessToken(self.token['access_token'], int(self.token['expires_on'])) ValueError: invalid literal for int() with base 10: '11/19/2021 10:40:07 +00:00'

That should be fixed with this modification ;)

Regards Marc.

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 18, 2021

Build to Cloud Shell: 12/03/2021 Official Release: 12/07/2021

@jiasli
Copy link
Member Author

jiasli commented Nov 19, 2021

@Maarc-D, your ValueError: invalid literal for int() with base 10: '11/19/2021 10:40:07 +00:00' issue is resolved by #20215.

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