Skip to content

Correct expires_on values of tokens from AzureCliCredential #14331

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.

Sign up for GitHub

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

Merged
chlowell merged 3 commits into from
Oct 7, 2020
Merged

Correct expires_on values of tokens from AzureCliCredential #14331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d77a9e3
Correct expires_on values of tokens from AzureCliCredential
chlowell Oct 7, 2020
7c8737f
update package version
chlowell Oct 7, 2020
7d40be2
changelog links to issue
chlowell Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Release History

## 1.4.1 (2020-10-07)
### Fixed
- `AzureCliCredential.get_token` correctly sets token expiration time,
preventing clients from using expired tokens
([#14345](https://github.com/Azure/azure-sdk-for-python/issues/14345))

## 1.4.0 (2020-08-10)
### Added
- `DefaultAzureCredential` uses the value of environment variable
Expand Down
15 changes: 10 additions & 5 deletions sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import platform
import re
import sys
import time
from typing import TYPE_CHECKING

import subprocess
Expand Down Expand Up @@ -68,14 +69,18 @@ def get_token(self, *scopes, **kwargs): # pylint:disable=no-self-use,unused-arg
def parse_token(output):
"""Parse output of 'az account get-access-token' to an AccessToken.

In particular, convert the CLI's "expiresOn" value, the string representation of a naive datetime, to epoch seconds.
In particular, convert the "expiresOn" value to epoch seconds. This value is a naive local datetime as returned by
datetime.fromtimestamp.
"""
try:
token = json.loads(output)
parsed_expires_on = datetime.strptime(token["expiresOn"], "%Y-%m-%d %H:%M:%S.%f")

# calculate seconds since the epoch; parsed_expires_on is naive
expires_on = (parsed_expires_on - datetime.fromtimestamp(0)).total_seconds()
dt = datetime.strptime(token["expiresOn"], "%Y-%m-%d %H:%M:%S.%f")
if hasattr(dt, "timestamp"):
# Python >= 3.3
expires_on = dt.timestamp()
Copy link
Member

@jiasli jiasli Oct 12, 2020

Choose a reason for hiding this comment

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

I don't really understand the original issue.

I tested on a UTC+8 machine and both forms return the same result. datetime.fromtimestamp(0) also returns a naive datetime of local time zone.

from datetime import datetime

dt = datetime.strptime("2020-10-12 11:24:00.0000", "%Y-%m-%d %H:%M:%S.%f")
print((dt - datetime.fromtimestamp(0)).total_seconds())
print(dt.timestamp())
print(datetime.fromtimestamp(0))

Output:

1602473040.0
1602473040.0
1970-01-01 08:00:00

Copy link
Member Author

@chlowell chlowell Oct 12, 2020

Choose a reason for hiding this comment

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

A naive calculation doesn't consider daylight saving time. When daylight saving time is in effect, as it is today in Seattle, you see this:

>>> dt = datetime.strptime("2020-10-12 11:24:00.0000", "%Y-%m-%d %H:%M:%S.%f")
>>> (dt - datetime.fromtimestamp(0)).total_seconds() - dt.timestamp()
3600.0

Copy link
Member

@jiasli jiasli Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. I see. Interesting. We don't have daylight saving time. 😆

Copy link
Member

@jiasli jiasli Oct 13, 2020

Choose a reason for hiding this comment

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

Perhaps CLI should have stuck with UTC from the very beginning.

Copy link
Member Author

@chlowell chlowell Oct 13, 2020

Choose a reason for hiding this comment

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

It's too late to change "expiresOn", but a new field like "expiresOnUTC" could solve the problem.

Copy link
Member

@jiasli jiasli Sep 18, 2023

Choose a reason for hiding this comment

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

To add a little bit more context, datetime.strptime("2020-10-12 11:24:00.0000", "%Y-%m-%d %H:%M:%S.%f") returns a naive datetime, so dt - datetime.fromtimestamp(0) doesn't take daylight saving time into consideration.

Meanwhile, according to https://docs.python.org/3/library/datetime.html#datetime.datetime.timestamp,

Naive datetime instances are assumed to represent local time and this method relies on the platform C mktime() function to perform the conversion.

so datetime.timestamp() will take daylight saving time into consideration, thus being 3600 seconds smaller than the result of dt - datetime.fromtimestamp(0).

else:
# taken from Python 3.5's datetime.timestamp()
expires_on = time.mktime((dt.year, dt.month, dt.day, dt.hour, dt.minute, dt.second, -1, -1, -1))

return AccessToken(token["accessToken"], int(expires_on))
except (KeyError, ValueError):
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/azure-identity/azure/identity/_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.
# ------------------------------------
VERSION = "1.4.0"
VERSION = "1.4.1"
7 changes: 3 additions & 4 deletions sdk/identity/azure-identity/tests/test_cli_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,10 @@ def test_get_token():
"""The credential should parse the CLI's output to an AccessToken"""

access_token = "access token"
valid_seconds = 42
expected_expires_on = 1602015811
successful_output = json.dumps(
{
# expiresOn is a naive datetime representing valid_seconds from the epoch
"expiresOn": datetime.fromtimestamp(valid_seconds).strftime("%Y-%m-%d %H:%M:%S.%f"),
"expiresOn": datetime.fromtimestamp(expected_expires_on).strftime("%Y-%m-%d %H:%M:%S.%f"),
"accessToken": access_token,
"subscription": "some-guid",
"tenant": "some-guid",
Expand All @@ -68,7 +67,7 @@ def test_get_token():

assert token.token == access_token
assert type(token.expires_on) == int
assert token.expires_on == valid_seconds
assert token.expires_on == expected_expires_on


def test_cli_not_installed_linux():
Expand Down
7 changes: 3 additions & 4 deletions sdk/identity/azure-identity/tests/test_cli_credential_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,10 @@ async def test_get_token():
"""The credential should parse the CLI's output to an AccessToken"""

access_token = "access token"
valid_seconds = 42
expected_expires_on = 1602015811
successful_output = json.dumps(
{
# expiresOn is a naive datetime representing valid_seconds from the epoch
"expiresOn": datetime.fromtimestamp(valid_seconds).strftime("%Y-%m-%d %H:%M:%S.%f"),
"expiresOn": datetime.fromtimestamp(expected_expires_on).strftime("%Y-%m-%d %H:%M:%S.%f"),
"accessToken": access_token,
"subscription": "some-guid",
"tenant": "some-guid",
Expand All @@ -93,7 +92,7 @@ async def test_get_token():

assert token.token == access_token
assert type(token.expires_on) == int
assert token.expires_on == valid_seconds
assert token.expires_on == expected_expires_on


async def test_cli_not_installed_linux():
Expand Down