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

AzureCliCredential to treat datetime without TZ info as local time. #5179

Merged
merged 12 commits into from
Dec 16, 2023
2 changes: 2 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

- [[#5075]](https://github.com/Azure/azure-sdk-for-cpp/issues/5075) `AzureCliCredential` assumes token expiration time without local time zone adjustment.

### Other Changes

- [[#5141]](https://github.com/Azure/azure-sdk-for-cpp/issues/5141) Added error response details to the Identity exceptions thrown when the authority host returns error response.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ namespace Azure { namespace Identity {
protected:
#endif
virtual std::string GetAzCommand(std::string const& scopes, std::string const& tenantId) const;
virtual int GetLocalTimeToUtcDiffSeconds() const;
};

}} // namespace Azure::Identity
23 changes: 22 additions & 1 deletion sdk/identity/azure-identity/src/azure_cli_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#include <azure/core/internal/unique_handle.hpp>
#include <azure/core/platform.hpp>

#include <chrono>
#include <cstdio>
#include <ctime>
#include <stdexcept>
#include <thread>
#include <type_traits>
Expand Down Expand Up @@ -127,6 +129,25 @@ std::string AzureCliCredential::GetAzCommand(std::string const& scopes, std::str
return command;
}

int AzureCliCredential::GetLocalTimeToUtcDiffSeconds() const
{
#ifdef _MSC_VER
#pragma warning(push)
// warning C4996: 'localtime': This function or variable may be unsafe. Consider using localtime_s
// instead.
#pragma warning(disable : 4996)
#endif
// LCOV_EXCL_START
auto const timeTNow = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());

return static_cast<int>(
std::difftime(std::mktime(std::localtime(&timeTNow)), std::mktime(std::gmtime(&timeTNow))));
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
// LCOV_EXCL_STOP
#ifdef _MSC_VER
#pragma warning(pop)
#endif
}

namespace {
std::string RunShellCommand(
std::string const& command,
Expand Down Expand Up @@ -154,7 +175,7 @@ AccessToken AzureCliCredential::GetToken(
try
{
return TokenCredentialImpl::ParseToken(
azCliResult, "accessToken", "expiresIn", "expiresOn");
azCliResult, "accessToken", "expiresIn", "expiresOn", GetLocalTimeToUtcDiffSeconds());
}
catch (json::exception const&)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ namespace Azure { namespace Identity { namespace _detail {
* @param expiresOnPropertyName Name of a property in the JSON object that represents token
* expiration as absolute date-time stamp. Can be empty, in which case no attempt to parse it is
* made.
* @param utcDiffSeconds Optional. If not 0, it represents the difference between the
* UTC and a desired time zone, in seconds. Then, should an RFC3339 timestamp come without a
* time zone information, a corresponding time zone offset will be applied to such timestamp.
* No credential other than AzureCliCredential (which gets timestamps as local time without time
* zone) should need to set it.
*
* @return A successfully parsed access token.
*
Expand All @@ -79,7 +84,8 @@ namespace Azure { namespace Identity { namespace _detail {
std::string const& jsonString,
std::string const& accessTokenPropertyName,
std::string const& expiresInPropertyName,
std::string const& expiresOnPropertyName);
std::string const& expiresOnPropertyName,
int utcDiffSeconds = 0);

/**
* @brief Holds `#Azure::Core::Http::Request` and all the associated resources for the HTTP
Expand Down
35 changes: 32 additions & 3 deletions sdk/identity/azure-identity/src/token_credential_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
#include <azure/core/url.hpp>

#include <chrono>
#include <iomanip>
#include <limits>
#include <map>
#include <sstream>

using Azure::Identity::_detail::TokenCredentialImpl;

Expand Down Expand Up @@ -192,13 +194,39 @@ std::string TokenAsDiagnosticString(
+ "\' property.\nSee Azure::Core::Diagnostics::Logger for details"
" (https://aka.ms/azsdk/cpp/identity/troubleshooting).");
}

std::string TimeZoneOffsetAsString(int utcDiffSeconds)
{
if (utcDiffSeconds == 0)
{
return {};
}

std::ostringstream os;
if (utcDiffSeconds >= 0)
{
os << '+';
}
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
else
{
os << '-';
utcDiffSeconds = -utcDiffSeconds;
}

os << std::setw(2) << std::setfill('0') << (utcDiffSeconds / (60 * 60)) << ':';
os << std::setw(2) << std::setfill('0') << ((utcDiffSeconds % (60 * 60)) / 60);

return os.str();
}

} // namespace

AccessToken TokenCredentialImpl::ParseToken(
std::string const& jsonString,
std::string const& accessTokenPropertyName,
std::string const& expiresInPropertyName,
std::string const& expiresOnPropertyName)
std::string const& expiresOnPropertyName,
int utcDiffSeconds)
{
json parsedJson;
try
Expand Down Expand Up @@ -309,13 +337,14 @@ AccessToken TokenCredentialImpl::ParseToken(
}
}

auto const tzOffsetStr = TimeZoneOffsetAsString(utcDiffSeconds);
if (expiresOn.is_string())
{
auto const expiresOnAsString = expiresOn.get<std::string>();
for (auto const& parse : {
std::function<DateTime(std::string const&)>([](auto const& s) {
std::function<DateTime(std::string const&)>([&](auto const& s) {
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
// 'expires_on' as RFC3339 date string (absolute timestamp)
return DateTime::Parse(s, DateTime::DateFormat::Rfc3339);
return DateTime::Parse(s + tzOffsetStr, DateTime::DateFormat::Rfc3339);
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
}),
std::function<DateTime(std::string const&)>([](auto const& s) {
// 'expires_on' as numeric string (posix time representing an absolute timestamp)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ std::string EchoCommand(std::string const text)
class AzureCliTestCredential : public AzureCliCredential {
private:
std::string m_command;
int m_localTimeToUtcDiffSeconds = 0;

std::string GetAzCommand(std::string const& resource, std::string const& tenantId) const override
{
Expand All @@ -60,6 +61,8 @@ class AzureCliTestCredential : public AzureCliCredential {
return m_command;
}

int GetLocalTimeToUtcDiffSeconds() const override { return m_localTimeToUtcDiffSeconds; }
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

public:
explicit AzureCliTestCredential(std::string command) : m_command(std::move(command)) {}

Expand All @@ -80,6 +83,8 @@ class AzureCliTestCredential : public AzureCliCredential {

decltype(m_tenantId) const& GetTenantId() const { return m_tenantId; }
decltype(m_cliProcessTimeout) const& GetCliProcessTimeout() const { return m_cliProcessTimeout; }

void SetLocalTimeToUtcDiffSeconds(int diff) { m_localTimeToUtcDiffSeconds = diff; }
};
} // namespace

Expand Down Expand Up @@ -570,6 +575,40 @@ TEST(AzureCliCredential, StrictIso8601TimeFormat)
DateTime::Parse("2022-08-24T00:43:08.000000Z", DateTime::DateFormat::Rfc3339));
}

TEST(AzureCliCredential, LocalTime)
{
constexpr auto Token = "{\"accessToken\":\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\","
"\"expiresOn\":\"2023-12-07 00:43:08\"}";
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

{
AzureCliTestCredential azCliCred(EchoCommand(Token));
azCliCred.SetLocalTimeToUtcDiffSeconds(-28800); // Redmond (no DST)

TokenRequestContext trc;
trc.Scopes.push_back("https://storage.azure.com/.default");
auto const token = azCliCred.GetToken(trc, {});

EXPECT_EQ(token.Token, "ABCDEFGHIJKLMNOPQRSTUVWXYZ");

EXPECT_EQ(
token.ExpiresOn, DateTime::Parse("2023-12-07T08:43:08Z", DateTime::DateFormat::Rfc3339));
}

{
AzureCliTestCredential azCliCred(EchoCommand(Token));
azCliCred.SetLocalTimeToUtcDiffSeconds(7200); // Kyiv (no DST)

TokenRequestContext trc;
trc.Scopes.push_back("https://storage.azure.com/.default");
auto const token = azCliCred.GetToken(trc, {});

EXPECT_EQ(token.Token, "ABCDEFGHIJKLMNOPQRSTUVWXYZ");

EXPECT_EQ(
token.ExpiresOn, DateTime::Parse("2023-12-06T22:43:08Z", DateTime::DateFormat::Rfc3339));
}
}

TEST(AzureCliCredential, Diagnosability)
{
{
Expand Down