Correct expires_on values of tokens from AzureCliCredential#14331
Correct expires_on values of tokens from AzureCliCredential#14331chlowell merged 3 commits intoAzure:hotfix/cli-credential-expires-onfrom
Conversation
sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py
Outdated
Show resolved
Hide resolved
42b1f5c to
d77a9e3
Compare
| dt = datetime.strptime(token["expiresOn"], "%Y-%m-%d %H:%M:%S.%f") | ||
| if hasattr(dt, "timestamp"): | ||
| # Python >= 3.3 | ||
| expires_on = dt.timestamp() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.0There was a problem hiding this comment.
Ah. I see. Interesting. We don't have daylight saving time. 😆
There was a problem hiding this comment.
Perhaps CLI should have stuck with UTC from the very beginning.
There was a problem hiding this comment.
It's too late to change "expiresOn", but a new field like "expiresOnUTC" could solve the problem.
There was a problem hiding this comment.
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).
|
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). |
The Azure CLI
get-access-tokencommand gives the token's expiry time in local time as produced bydatetime.fromtimestamp(here), with no timezone information. Because AzureCliCredential uses a naivedatetimeto convert this string to epoch seconds, it can provide a token with an incorrectexpires_onvalue, 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). Whendatetime.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 thefoldattribute described in PEP 495.Closes #14345