AzureCliCredential parses an incorrect token expiry time in multi-threaded Unix applications

[pencels]

Description

To get an access token, AzureCliCredential shells out to az account get-access-token and parses up the expiresOn string as a datetime value. Since az formats the datetime without a timezone offset, it assumes the offset with date::assume_local().

azure-sdk-for-rust/sdk/identity/src/token_credentials/azure_cli_credentials.rs

Lines 19 to 26 in f978185

	pub fn parse(s: &str) -> azure_core::Result<OffsetDateTime> {
	// expiresOn from azure cli uses the local timezone and needs to be converted to UTC
	let dt = PrimitiveDateTime::parse(s, FORMAT)
	.with_context(ErrorKind::DataConversion, || {
	format!("unable to parse expiresOn '{s}")
	})?;
	Ok(date::assume_local(&dt))
	}

date::assume_local() delegates to time's function UtcOffset::current_local_offset().

azure-sdk-for-rust/sdk/core/src/date/mod.rs

Lines 116 to 119 in f978185

	/// Assumes the local offset. Default to UTC if unable to get local offset.
	pub fn assume_local(date: &PrimitiveDateTime) -> OffsetDateTime {
	date.assume_offset(UtcOffset::current_local_offset().unwrap_or(UtcOffset::UTC))
	}

For most Unix systems, UtcOffset::current_local_offset() will return Ok(offset) for single-threaded applications, and Err(IndeterminateOffset) for multi-threaded applications, as defined by this function.

In my case, I am running my application on Ubuntu (WSL), and my application is multi-threaded. So this offset is always indeterminate.

(Aside: There's a whole conversation about thread-safety of accessing local time zone data, which is the reason why this behavior exists. If certain things change in the Rust ecosystem, then this problem may just resolve itself as simply as installing a new release of time.)

Because the offset is indeterminate, date::assume_local() will default the expiry time to being in the UTC time zone, when it should be in my local (PDT) time zone. This is obviously incorrect behavior, because the expiry time is now hours off from the actual time it should represent.

This can manifest as bugs further down the line, e.g. in AutoRefreshingTokenCredential where recently cached tokens are refreshed constantly because they are viewed as expired. Each token refresh adds consistent overhead to SDK calls. In particular, this led my calls through SDK clients to take approximately ~1.3s each rather than an expected ~90ms each when the token was only refreshed as needed.

Workarounds/Solutions

• As users of the SDK, what we can do to mitigate this:
  • (RECOMMENDED) Use other credential types. This will avoid the issue, simply because other credential types don't interact with this code.
  • (NOT recommended) Manually override the thread-count check by using the set_soundness() function provided by time. This requires some understanding of the thread-safety of your application and probably isn't worth the effort to do correctly.
• As SDK authors, what we can do to fix this:
  • Wait for the Rust ecosystem to figure out a way to manage thread-safe access of local time information, then consume this new thread-safe interface.
  • Push for changes in the Azure CLI output. It seems az-cli was looking into representing the expiry time as an integer timestamp instead (az account get-access-token: Use epoch expiresOn/expires_on azure-cli#19700). If implemented, the AzureCliCredential would just have to deserialize the epoch timestamp, circumventing the need to determine the system's local offset. However, I haven't seen much activity on this issue, so I'm not sure what the timelines are there.
  • We could also try to use set_soundness() in the SDK itself, but this is probably unsafe. Again, not recommended.

Relevant Issues/PRs

• az account get-access-token: Use epoch expiresOn/expires_on azure-cli#19700
• {Profile} az account get-access-token: Show expiresOn for managed identity azure-cli#20219
• Time 0.2.23 fails to determine local offset time-rs/time#296
• The call to localtime_r may be unsound time-rs/time#293

[demoray]

@cataggar, with the recent changes made to chrono, we might want to revisit our migration from chrono to time as it relates to this. Specifically, chrono no longer relies on libc::localtime_r, instead replies on code originally from the tz-rs crate to handle this in a thread-safe fashion.

Ref: https://github.com/chronotope/chrono/releases/tag/v0.4.30

[cataggar]

I think it is best that the Azure CLI pass the unix timestamp along.
I opened Azure/azure-cli#27410 to see if we could this going again.

[cataggar]

My PR to pass the unix timestamp was replaced with Azure/azure-cli#27476

[demoray]

This has been mitigated in 0.16.2, released earlier today. In the future, we should make use of the az-cli feature that was linked earlier if the functionality is available.

For existing az-cli instances, we use tz-rs to attempt to identify and resolve the timezone.