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

az account get-access-token: Use epoch expiresOn/expires_on #19700

Closed
jiasli opened this issue Sep 27, 2021 · 7 comments · Fixed by #27476
Closed

az account get-access-token: Use epoch expiresOn/expires_on #19700

jiasli opened this issue Sep 27, 2021 · 7 comments · Fixed by #27476
Assignees
Labels
Milestone

Comments

@jiasli
Copy link
Member

jiasli commented Sep 27, 2021

Context

Currently, the expiresOn property in the output of az account get-access-token is a string representing the local time:

{
  "accessToken": "eyJ0eXAiOiJ...",
  "expiresOn": "2021-09-27 16:16:30.921462",  <<<
  "subscription": "0b1f6471-1bf0-4dda-aec3-cb9272f09590",
  "tenant": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a",
  "tokenType": "Bearer"
}

This inherits from the ADAL behavior: https://github.com/AzureAD/azure-activedirectory-library-for-python/blob/35f9e003bed47936a1df3c6eb696691dc1fa91c3/adal/oauth2_client.py#L187

This causes various time computation problems and difficulties, especially for daylight saving time, such as

Other tools or services are already using an integer to represent the epoch time:

This approach is universal and eliminates all necessary complexity.

Proposal

There are possible approaches:

  1. Make a BREAKING CHANGE to convert the expiresOn in the output of az account get-access-token to an integer representing the epoch time. This can be done during MSAL migration: ADAL to MSAL migration #18944
  2. Introduce another property expires_on. This will not be a BREAKING CHANGE, but introduces naming inconsistency as other properties uses camelCase, but expires_on is snake_case.
  3. Introduce another properties like expiresOnEpoch, but naming like this is non-conventional.
@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Sep 27, 2021
@yonzhan yonzhan added the Account az login/account label Sep 27, 2021
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Sep 27, 2021
@yonzhan yonzhan added this to the Backlog milestone Sep 27, 2021
@jiasli jiasli changed the title az account get-access-token: Use integer expiresOn/expires_on az account get-access-token: Use epoch expiresOn/expires_on Oct 7, 2021
@jiasli jiasli modified the milestones: Backlog, Nov 2021 (2021-12-07) Nov 6, 2021
@jiasli
Copy link
Member Author

jiasli commented Sep 18, 2023

Another possible approach is to

  1. Add --epoch-expires-on argument which changes the format of expiresOn to epoch and show a warning to instruct downstream programs to use it
  2. After some time, return epoch expiresOn as default (BREAKING CHANGE) and make --epoch-expires-on argument a no-op
  3. After some time, remove --epoch-expires-on (BREAKING CHANGE)

A downside with this approach is that it breaks interoperability between certain combinations of Azure CLI and Azure Identity. For example:

  • After Azure CLI switches to epoch expiresOn, old Azure Identity depending on local expiresOn will break
  • New Azure Identity depending on epoch expiresOn will fail when old Azure CLI returns local expiresOn

The bottom line is that we can't live with --epoch-expires-on forever, as this will make Azure CLI carry too much historical burden. --epoch-expires-on must be made the default at some time in the future.

@jiasli
Copy link
Member Author

jiasli commented Sep 18, 2023

Now that #27410 brings this discussion back to life, I would personally prefer introducing epoch expires_on.

Even though it has naming inconsistency with other properties of Azure CLI output, it is consistent with other services or tools, such as

so this "inconsistency with other properties of Azure CLI output" should be acceptable.

In fact, a deeper reason for the inconsistency comes from the fact that ARM uses camelCase as JSON keys, while authentication-related services, such as AAD and managed identity, use snake_case as JSON keys. When they meet in Azure CLI, Azure CLI must coordinate them.

A big advantage with this approach is that it introduces zero breaking change.

On the other hand, expiresOnEpoch is not used by any other service and introduces lots of unnecessary complexity.

@jiasli
Copy link
Member Author

jiasli commented Sep 20, 2023

Daylight Saving Time problem

The current local datetime string format expiresOn causes problem when Daylight Saving Time ends.

According to https://aa.usno.navy.mil/faq/daylight_time

On the first Sunday in November, clocks are set back one hour at 2:00 a.m. local Daylight Saving Time (which becomes 1:00 a.m. local standard time).

Consider

  1. 2023-11-05 01:15:00.00000 fold=0
  2. 2023-11-05 01:15:00.00000 fold=1

As there is no way for expiresOn to distinguish whether the time is folded, even at the second 01:15, expiresOn represents the first 01:15, and a downstream application parsing expiresOn will treat it as the first 01:15 and think the token has expired.

A code snippet demonstrating this:

from datetime import datetime
from zoneinfo import ZoneInfo

tz = ZoneInfo("America/Los_Angeles")

dt = datetime(2023, 11, 5, 1, 15, tzinfo=tz, fold=0)

epoch0 = dt.timestamp()         # The first 01:15
epoch1 = dt.timestamp() + 3600  # The second 01:15

dt0 = datetime.fromtimestamp(epoch0, tz=tz)
dt1 = datetime.fromtimestamp(epoch1, tz=tz)

print(epoch0, repr(dt0), dt0)
# output: 1699172100.0 datetime.datetime(2023, 11, 5, 1, 15, tzinfo=zoneinfo.ZoneInfo(key='America/Los_Angeles')) 2023-11-05 01:15:00-07:00

print(epoch1, repr(dt1), dt1)
# output: 1699175700.0 datetime.datetime(2023, 11, 5, 1, 15, fold=1, tzinfo=zoneinfo.ZoneInfo(key='America/Los_Angeles')) 2023-11-05 01:15:00-08:00

Notice the second 01:15 can be expressed by either:

  • Using the fold attribute
  • Using the time zone -07:00/-08:00

Because the current format %Y-%m-%d %H:%M:%S.%f contains neither of them, 2023-11-05 01:15:00.00000 is ambiguous during this folded hour.

When the timezone of the machine is set to Pacific Time, there will be 7200 seconds between 01:15 and 02:15:

dt0 = datetime.strptime('2023-11-05 01:15:00.00000', "%Y-%m-%d %H:%M:%S.%f")
dt1 = datetime.strptime('2023-11-05 02:15:00.00000', "%Y-%m-%d %H:%M:%S.%f")
print(dt1.timestamp() - dt0.timestamp())
# output: 7200.0

@jiasli
Copy link
Member Author

jiasli commented Sep 21, 2023

Year 2038 problem

Returning expires_on as int epoch timestamp still faces the Year 2038 problem.

I use this script to determine the max timestamp supported by Python:

from datetime import datetime, timezone
import traceback
import time

max_epoch = time.time()
seconds_of_one_year = 365 * 24 * 3600
dt = None

while True:
    try:
        dt = datetime.fromtimestamp(max_epoch, tz=timezone.utc)
        max_epoch += seconds_of_one_year
    except:
        traceback.print_exc()
        break

print(dt)

On Windows, the max year is 3000:

Traceback (most recent call last):
  File "D:\cli\testproj\main.py", line 11, in <module>
    dt = datetime.fromtimestamp(max_epoch, tz=timezone.utc)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 22] Invalid argument
3000-01-27 07:50:09.968254+00:00

On Linux, the max year is 9999:

Traceback (most recent call last):
  File "/home/user2/test.py", line 11, in <module>
    dt = datetime.fromtimestamp(max_epoch, tz=timezone.utc)
ValueError: year 10000 is out of range
9999-06-04 07:50:28.012756+00:00

Even if Azure CLI can return an expires_on up to year 3000 or 9999, downstream applications that only have 32-bit precision integer will face error when expires_on reaches $2^{31} − 1$ on 2038-01-19 03:14:07 UTC, but it's the downstream applications' responsibility to handle that.

See https://stackoverflow.com/questions/46133223/maximum-value-of-timestamp

@jiasli
Copy link
Member Author

jiasli commented Oct 27, 2023

Another reason for returning expires_on as POSIX timestamp is because a local datetime causes issues in Azure Rust SDK: Azure/azure-sdk-for-rust#1371.

@antkmsft
Copy link
Member

Do you think it could be possible to keep the token expiration in RFC3339 format and not Unix epoch, but to convert to UTC and add Z in the end? - "expiresOn": "2021-09-27 20:16:30.921462Z" clearly means "the timestamp is UTC".

@jiasli
Copy link
Member Author

jiasli commented Oct 30, 2023

@antkmsft, we chose to use Unix epoch because many other Azure tools or services use it, such as MSAL, Azure SDKs, Managed Identity, as explained in #19700 (comment).

Changing expiresOn from local datetime to UTC would be a breaking change and break countless downstream applications.

If you need UTC datetime, you may convert expires_on by yourself, such as

$ echo 1347517370 | python -c 'import datetime, sys; print(datetime.datetime.utcfromtimestamp(int(sys.stdin.read())).strftime("%Y-%m-%d %H:%M:%SZ"))'
2012-09-13 06:22:50Z

See https://stackoverflow.com/questions/12400256/converting-epoch-time-into-the-datetime

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

Successfully merging a pull request may close this issue.

3 participants