Azure-Core: If no timezone information is present in string, assume local time and convert to UTC

[janbernloehr]

This PR proposes a fix for #5075. There might be side-effects, though. Feedback welcome.

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

• C++ Guidelines
• Doxygen docs
• Unit tests
• No unwanted commits/changes
• Descriptive title/description
  • PR is single purpose
  • Related issue listed
• Comments in source
• No typos
• Update changelog
• Not work-in-progress
• External references or docs updated
• Self review of PR done
• Any breaking changes?

[github-actions]

Thank you for your contribution @janbernloehr! We will review the pull request and get back to you soon.

[antkmsft]

Thank you, @janbernloehr!
Current behavior existed since the beginning, and it is hard to tell right away, how, if, and where it would affect us or our customers.
This looks like a breaking change - previously a user would parse 2023-10-26T18:18:18 and get UTC value, now they'll get a different value.

FWIW, current behavior is consistent with .NET:

Console.WriteLine(DateTime.Parse("2023-10-26T18:18:18").ToLocalTime()); // When ran in PST time zone, prints "Oct 26, 2023 11:18:18".

I think #5075 should be fixed tactically in the Azure CLI credential - it could be that DateTime::Parse() gets something like a treatNoTimeZoneAsLocal = false third parameter, or something like that. And then the AzureCLI credential passes that flag to the TokenCredentialImpl, and then its token parser uses that flag when parsing the token expiration time.

cc @ahsonkhan

[LarryOsterman]

One minor note: RFC 3339 requires a timezone element.

Our parser appears to allow RFC 3339 elements without a timezone element, but it is a non-standard extension. We certainly should never generate such a time.

[janbernloehr]

One minor note: RFC 3339 requires a timezone element.

Our parser appears to allow RFC 3339 elements without a timezone element, but it is a non-standard extension. We certainly should never generate such a time.

It could probably also be fixed in az cli by creating a string complying with RFC 3339

these might be related:

• az account get-access-token: Use epoch expiresOn/expires_on azure-cli#19700
• [Profile] az account get-access-token: Return expires_on as POSIX timestamp azure-cli#27476
• use tz::TimeZone for az-cli time conversion on unix platforms azure-sdk-for-rust#1431

[antkmsft]

Thank you, @janbernloehr - these are great links and the context, I even left one possible idea in the first one.
But I don't think this SDK will be able to enjoy that feature they may add in the future version, because our customer may have the old version of Azure CLI extended, and for that, we will need to not be using any new command line switches added in the older versions, and instead we would need to be treating an RFC3339 timestamp without Z or +NNNN in the end as local time, but only when parsing the token expiration from az account get-access-token output, and not in any other scope.

[jiasli]

Hi @janbernloehr, @antkmsft, I am the owner of Azure CLI command az account get-access-token. Returning expiresOn as a local datetime is only a legacy from the ADAL age. We would not make any change on that field.

az account get-access-token will start returning expires_on as a POSIX timestamp (Azure/azure-cli#27476) since Azure CLI 2.54.0 which will be released on 2023-11-14, and we would highly recommend using that new field as it is timezone-irrelevant and consistent across various tools and services - MSAL, Azure SDKs, Managed Identity.

[antkmsft]

Thank you @jiasli - the Azure SDK for C++'s current implementation would successfully be able to parse Unix timestamp (

azure-sdk-for-cpp/sdk/identity/azure-identity/src/token_credential_impl.cpp

Lines 292 to 327 in 50c1851

	if (expiresOn.is_number_unsigned())
	{
	try
	{
	// 'expires_on' as number (posix time representing an absolute timestamp)
	auto const value = expiresOn.get<std::int64_t>();
	if (value <= MaxPosixTimestamp)
	{
	accessToken.ExpiresOn = PosixTimeConverter::PosixTimeToDateTime(value);
	return accessToken;
	}
	}
	catch (std::exception const&)
	{
	// expiresIn.get<std::int64_t>() has thrown, we may throw later.
	}
	}
	if (expiresOn.is_string())
	{
	auto const expiresOnAsString = expiresOn.get<std::string>();
	for (auto const& parse : {
	std::function<DateTime(std::string const&)>([](auto const& s) {
	// 'expires_on' as RFC3339 date string (absolute timestamp)
	return DateTime::Parse(s, DateTime::DateFormat::Rfc3339);
	}),
	std::function<DateTime(std::string const&)>([](auto const& s) {
	// 'expires_on' as numeric string (posix time representing an absolute timestamp)
	return PosixTimeConverter::PosixTimeToDateTime(
	ParseNumericExpiration(s, MaxPosixTimestamp));
	}),
	std::function<DateTime(std::string const&)>([](auto const& s) {
	// 'expires_on' as RFC1123 date string (absolute timestamp)
	return DateTime::Parse(s, DateTime::DateFormat::Rfc1123);
	}),
	})

), should it replace the RFC3339 string, no code change is needed.

AzureSDK for .NET would break - https://github.com/Azure/azure-sdk-for-net/blob/ba1e6df24131bfc88619c12c4c592933f78fcd18/sdk/identity/Azure.Identity/src/Credentials/AzureCliCredential.cs#L236, we should communicate this to them. Possibly, some other SDKs may get broken by this change as well.

cc @ahsonkhan @schaabs @scottaddie @azure-sdk-write-identity

[scottaddie]

@jiasli Which version number will contain this new expires_on field?

[jiasli]

@jiasli Which version number will contain this new expires_on field?

@scottaddie, I have updated my comment #5076 (comment) to include that information.

[jiasli]

@antkmsft, I provided more information in Azure/azure-cli#27476. We are not going to make any change on expiresOn, but introducing a new POSIX timestamp expires_on, so it won't breaking any existing SDK.

[antkmsft]

#5179 would treat datetimes it gets from azcli as local time, if no time zone information is present.

[antkmsft]

Fixed in #5179