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

Correct expires_on values of tokens from AzureCliCredential #14331

Merged

Conversation

chlowell
Copy link
Member

@chlowell chlowell commented Oct 7, 2020

The Azure CLI get-access-token command gives the token's expiry time in local time as produced by datetime.fromtimestamp (here), with no timezone information. Because AzureCliCredential uses a naive datetime to convert this string to epoch seconds, it can provide a token with an incorrect expires_on value, causing a client to send an expired access token.

This PR fixes the test which should have detected this, and has the credential parse the CLI's output with the datetime.timestamp() method when available (it was added in Python 3.3). When datetime.timestamp() is unavailable, e.g. on Python 2.7, the credential falls back to its implementation in 3.5 (here). I chose 3.5's implementation despite 3.6's improvements around clock shifts (e.g. daylight saving time) because those improvements require the fold attribute described in PEP 495.

Closes #14345

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Oct 7, 2020
@chlowell chlowell requested a review from mccoyp October 7, 2020 18:35
@chlowell chlowell requested a review from schaabs as a code owner October 7, 2020 18:35
mccoyp
mccoyp previously approved these changes Oct 7, 2020
@chlowell chlowell changed the base branch from master to hotfix/cli-credential-expires-on October 7, 2020 20:50
@chlowell chlowell requested a review from mccoyp October 7, 2020 20:51
@chlowell chlowell merged commit 4b017b6 into Azure:hotfix/cli-credential-expires-on Oct 7, 2020
@chlowell chlowell deleted the cli-expires-on branch October 7, 2020 21:21
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

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

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

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

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

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

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).

@jiasli
Copy link
Member

jiasli commented Oct 30, 2023

There is still one problem with this implementation - the local datetime string is not able to represent "fold" and Azure Identity won't be able to get the correct POSIX timestamp during the second 01:00 ~ 02:00 when Daylight Saving Time ends each year. More details: Azure/azure-cli#19700 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants