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

Add ADFS support for ClientSecretCredential #1947

Merged
merged 24 commits into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sdk/core/azure-core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,12 @@ set(
inc/azure/core/internal/diagnostics/log.hpp
inc/azure/core/internal/hkeyholder.hpp
inc/azure/core/internal/http/pipeline.hpp
inc/azure/core/internal/http/test_transport.hpp
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
inc/azure/core/internal/io/null_body_stream.hpp
inc/azure/core/internal/json/json_serializable.hpp
inc/azure/core/internal/json/json.hpp
inc/azure/core/internal/strings.hpp
inc/azure/core/internal/system_clock.hpp
inc/azure/core/io/body_stream.hpp
inc/azure/core/base64.hpp
inc/azure/core/case_insensitive_containers.hpp
Expand Down Expand Up @@ -106,6 +108,7 @@ set(
src/logger.cpp
src/operation_status.cpp
src/strings.cpp
src/system_clock.cpp
src/version.cpp
)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// SPDX-License-Identifier: MIT

#pragma once

#include <functional>

#include "azure/core/http/transport.hpp"

namespace Azure { namespace Core { namespace Http { namespace _internal {
class TestTransport : public HttpTransport {
public:
typedef std::function<std::unique_ptr<RawResponse>(Request& request, Context const& context)>
SendCallback;

private:
SendCallback m_sendCallback;

public:
TestTransport(SendCallback send) : m_sendCallback(send) {}

std::unique_ptr<RawResponse> Send(Request& request, Context const& context) override
{
return m_sendCallback(request, context);
}
};
}}}} // namespace Azure::Core::Http::_internal
34 changes: 34 additions & 0 deletions sdk/core/azure-core/inc/azure/core/internal/system_clock.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// SPDX-License-Identifier: MIT

#pragma once

#include <atomic>
#include <chrono>
#include <functional>

#include "azure/core/dll_import_export.hpp"
#include "azure/core/http/transport.hpp"

namespace Azure { namespace Core { namespace _internal {
class SystemClock {
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
public:
typedef std::function<std::chrono::system_clock::time_point()> NowCallback;

private:
static_assert(ATOMIC_BOOL_LOCK_FREE == 2, "atomic<bool> must be lock-free");
static AZ_CORE_DLLEXPORT std::atomic<bool> g_isOverridden;
static NowCallback::result_type OverriddenNow();

static void Override(NowCallback now);

public:
SystemClock(NowCallback now) { Override(now); }
~SystemClock() { Override(nullptr); }

static std::chrono::system_clock::time_point Now()
{
return g_isOverridden ? OverriddenNow() : std::chrono::system_clock::now();
}
};
}}} // namespace Azure::Core::_internal
16 changes: 15 additions & 1 deletion sdk/core/azure-core/src/datetime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ DateTime GetSystemClockEpoch()
static_cast<int8_t>(systemClockEpochUtcStructTm->tm_sec));
}

DateTime GetMaxDateTime()
{
auto const systemClockMax = std::chrono::duration_cast<DateTime::clock::duration>(
std::chrono::system_clock::time_point::max().time_since_epoch())
.count();

auto const systemClockEpoch = GetSystemClockEpoch().time_since_epoch().count();

constexpr auto repMax = std::numeric_limits<DateTime::clock::duration::rep>::max();

return DateTime(DateTime::time_point(
DateTime::duration(systemClockMax + std::min(systemClockEpoch, (repMax - systemClockMax)))));
}
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

template <typename T>
void ValidateDateElementRange(
T value,
Expand Down Expand Up @@ -409,7 +423,7 @@ DateTime::DateTime(
DateTime::operator std::chrono::system_clock::time_point() const
{
static DateTime SystemClockMin(std::chrono::system_clock::time_point::min());
static DateTime SystemClockMax(std::chrono::system_clock::time_point::max());
static DateTime SystemClockMax(GetMaxDateTime());

auto outOfRange = 0;
if (*this < SystemClockMin)
Expand Down
29 changes: 29 additions & 0 deletions sdk/core/azure-core/src/system_clock.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// SPDX-License-Identifier: MIT

#include "azure/core/internal/system_clock.hpp"

#include <mutex>
#include <shared_mutex>

using namespace Azure::Core::_internal;

std::atomic<bool> SystemClock::g_isOverridden(false);

namespace {
std::shared_timed_mutex g_mutex;
SystemClock::NowCallback g_now(nullptr);
} // namespace

SystemClock::NowCallback::result_type SystemClock::OverriddenNow()
{
std::shared_lock<std::shared_timed_mutex> lock(g_mutex);
return g_now();
}

void SystemClock::Override(SystemClock::NowCallback now)
{
std::unique_lock<std::shared_timed_mutex> lock(g_mutex);
g_now = now;
g_isOverridden = now != nullptr;
}
57 changes: 47 additions & 10 deletions sdk/identity/azure-identity/src/client_secret_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,45 @@

#include <azure/core/http/http.hpp>
#include <azure/core/internal/http/pipeline.hpp>
#include <azure/core/internal/system_clock.hpp>

#include <chrono>
#include <sstream>

namespace {
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
// Assumes !scopes.empty()
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
std::string FormatScopes(std::vector<std::string> const& scopes, bool asResource)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does asResource mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is internal function. You can only have single scope as resource, and when you want to build resource, you don't add scope suffix.

{
if (asResource && scopes.size() == 1)
{
auto resource = scopes[0];
constexpr char suffix[] = "/.default";
constexpr int suffixLen = sizeof(suffix) - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing -1. Add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this necessarily requires a comment, this is the part of the language - when an array is initialized from a literal, \0 is added to the array - https://en.cppreference.com/w/c/language/array_initialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this necessarily requires a comment

Source comments are rarely necessary. It's just meant for helpful hints for future maintainability when code changes, moves around. What happens if I change line 19 tomorrow to std::string, now line 20 is broken, if I don't update -1, correct?

It's up to your discretion, as the PR author, what's clear/obvious by just reading the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add it, but if we change it to std::string, suffixLen should be removed entirely, it is no longer necessary. So, the only context in which it makes sense, is when suffix is a C-string, so it is 0-terminated, so that "- 1" is needed.

But even without the "-1", sizeof(std::string()) is broken. It will be the size of the std::string class, not the length of the string. length() should be used.

auto const resourceLen = resource.length();

// If scopes[0] ends with '/.default', remove it.
if (resourceLen >= suffixLen
&& resource.find(suffix, resourceLen - suffixLen) != std::string::npos)
{
resource = resource.substr(0, resourceLen - suffixLen);
}

return Azure::Core::Url::Encode(resource);
}

auto scopesIter = scopes.begin();
auto scopesStr = Azure::Core::Url::Encode(*scopesIter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: re-add the comment here, that we assume scopes.size() > 0


auto const scopesEnd = scopes.end();
for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
{
scopesStr += std::string(" ") + Azure::Core::Url::Encode(*scopesIter);
}

return scopesStr;
}
} // namespace

using namespace Azure::Identity;

std::string const Azure::Identity::_detail::g_aadGlobalAuthority
Expand All @@ -27,24 +62,21 @@ Azure::Core::Credentials::AccessToken ClientSecretCredential::GetToken(
static std::string const errorMsgPrefix("ClientSecretCredential::GetToken: ");
try
{
auto const isAdfs = m_tenantId == "adfs";

Url url(m_options.AuthorityHost);
url.AppendPath(m_tenantId);
url.AppendPath("oauth2/v2.0/token");
url.AppendPath(isAdfs ? "oauth2/token" : "oauth2/v2.0/token");

std::ostringstream body;
body << "grant_type=client_credentials&client_id=" << Url::Encode(m_clientId)
<< "&client_secret=" << Url::Encode(m_clientSecret);

auto const& scopes = tokenRequestContext.Scopes;
if (!scopes.empty())
{
auto scopesIter = scopes.begin();
body << "&scope=" << Url::Encode(*scopesIter);

auto const scopesEnd = scopes.end();
for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
auto const& scopes = tokenRequestContext.Scopes;
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
if (!scopes.empty())
{
body << " " << *scopesIter;
body << "&scope=" << FormatScopes(scopes, isAdfs);
}
}

Expand All @@ -58,6 +90,11 @@ Azure::Core::Credentials::AccessToken ClientSecretCredential::GetToken(
request.SetHeader("Content-Type", "application/x-www-form-urlencoded");
request.SetHeader("Content-Length", std::to_string(bodyString.size()));

if (isAdfs)
{
request.SetHeader("Host", url.GetHost());
}

HttpPipeline httpPipeline(m_options, "Identity-client-secret-credential", "", {}, {});

std::shared_ptr<RawResponse> response = httpPipeline.Send(request, context);
Expand Down Expand Up @@ -150,7 +187,7 @@ Azure::Core::Credentials::AccessToken ClientSecretCredential::GetToken(

return {
std::string(responseBodyBegin + tokenBegin, responseBodyBegin + tokenEnd),
std::chrono::system_clock::now() + std::chrono::seconds(expiresInSeconds),
Azure::Core::_internal::SystemClock::Now() + std::chrono::seconds(expiresInSeconds),
};
}
catch (AuthenticationException const&)
Expand Down
1 change: 1 addition & 0 deletions sdk/identity/azure-identity/test/ut/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ add_executable (
macro_guard.cpp
main.cpp
simplified_header.cpp
client_secret_credential.cpp
)

if (MSVC)
Expand Down
149 changes: 149 additions & 0 deletions sdk/identity/azure-identity/test/ut/client_secret_credential.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// SPDX-License-Identifier: MIT

/**
* @brief makes sure azure/identity.hpp can be included.
*
* @remark This file will catch any issue while trying to use/include the identity.hpp header
*
*/

#include "azure/identity/client_secret_credential.hpp"

#include <azure/core/internal/http/test_transport.hpp>
#include <azure/core/internal/system_clock.hpp>
#include <azure/core/io/body_stream.hpp>

#include <gtest/gtest.h>

using namespace Azure::Identity;

namespace {

struct CredentialResult
{
struct RequestInfo
{
std::string AbsoluteUrl;
Azure::Core::CaseInsensitiveMap Headers;
std::string Body;
} Request;

Azure::Core::Credentials::AccessToken Response;
};

CredentialResult TestClientSecretCredential(
std::string const& tenantId,
std::string const& clientId,
std::string const& clientSecret,
ClientSecretCredentialOptions credentialOptions,
Azure::Core::Credentials::TokenRequestContext const& tokenRequestContext,
Azure::DateTime const& clockOverride,
std::string const& responseBody)
{
CredentialResult result;

auto responseVec = std::vector<uint8_t>(responseBody.begin(), responseBody.end());
credentialOptions.Transport.Transport
= std::make_shared<Azure::Core::Http::_internal::TestTransport>([&](auto request, auto) {
auto const bodyVec = request.GetBodyStream()->ReadToEnd(Azure::Core::Context());

result.Request
= {request.GetUrl().GetAbsoluteUrl(),
request.GetHeaders(),
std::string(bodyVec.begin(), bodyVec.end())};

auto response = std::make_unique<Azure::Core::Http::RawResponse>(
1, 1, Azure::Core::Http::HttpStatusCode::Ok, "OK");

response->SetBodyStream(std::make_unique<Azure::Core::IO::MemoryBodyStream>(responseVec));

return response;
});

ClientSecretCredential credential(tenantId, clientId, clientSecret, credentialOptions);

Azure::Core::_internal::SystemClock overriddenSystemClock(
[=]() { return static_cast<std::chrono::system_clock::time_point>(clockOverride); });

result.Response = credential.GetToken(tokenRequestContext, Azure::Core::Context());

return result;
}
} // namespace

TEST(ClientSecretCredential, Regular)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I see the test coverage for this change and azure-identity overall?

It's not visible here: https://dev.azure.com/azure-sdk/public/_build/results?buildId=808213&view=results

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how the code coverage integration works, but if you don't see it, it most likely was not enable for identity. We need a work item to enable it.

{
ClientSecretCredentialOptions options;
options.AuthorityHost = "https://authority.url/";
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
auto const actual = TestClientSecretCredential(
"01234567-89ab-cdef-fedc-ba8976543210",
"fedcba98-7654-3210-0123-456789abcdef",
"CLIENTSECRET",
options,
{{"https://resource.url/.default"}},
Azure::DateTime(2021, 1, 1, 0),
"{\"expires_in\":3600, \"access_token\":\"ACCESSTOKEN1\"}");

EXPECT_EQ(
actual.Request.AbsoluteUrl,
"https://authority.url/01234567-89ab-cdef-fedc-ba8976543210/oauth2/v2.0/token");

{
constexpr char expectedBody[] = "grant_type=client_credentials"
"&client_id=fedcba98-7654-3210-0123-456789abcdef"
"&client_secret=CLIENTSECRET"
"&scope=https%3A%2F%2Fresource.url%2F.default";

EXPECT_EQ(actual.Request.Body, expectedBody);

EXPECT_NE(actual.Request.Headers.find("Content-Length"), actual.Request.Headers.end());
EXPECT_EQ(
actual.Request.Headers.at("Content-Length"), std::to_string(sizeof(expectedBody) - 1));
}

EXPECT_NE(actual.Request.Headers.find("Content-Type"), actual.Request.Headers.end());
EXPECT_EQ(actual.Request.Headers.at("Content-Type"), "application/x-www-form-urlencoded");

EXPECT_EQ(actual.Response.Token, "ACCESSTOKEN1");
EXPECT_EQ(actual.Response.ExpiresOn, Azure::DateTime(2021, 1, 1, 1));
EXPECT_EQ(actual.Response.ExpiresOn.ToString(), Azure::DateTime(2021, 1, 1, 1).ToString());
}

TEST(ClientSecretCredential, AzureStack)
{
ClientSecretCredentialOptions options;
options.AuthorityHost = "https://authority.url/";
auto const actual = TestClientSecretCredential(
"adfs",
"fedcba98-7654-3210-0123-456789abcdef",
"CLIENTSECRET",
options,
{{"https://resource.url/.default"}},
Azure::DateTime(2021, 1, 1, 0),
"{\"expires_in\":3600, \"access_token\":\"ACCESSTOKEN1\"}");

EXPECT_EQ(actual.Request.AbsoluteUrl, "https://authority.url/adfs/oauth2/token");

{
constexpr char expectedBody[] = "grant_type=client_credentials"
"&client_id=fedcba98-7654-3210-0123-456789abcdef"
"&client_secret=CLIENTSECRET"
"&scope=https%3A%2F%2Fresource.url";

EXPECT_EQ(actual.Request.Body, expectedBody);

EXPECT_NE(actual.Request.Headers.find("Content-Length"), actual.Request.Headers.end());
EXPECT_EQ(
actual.Request.Headers.at("Content-Length"), std::to_string(sizeof(expectedBody) - 1));
}

EXPECT_NE(actual.Request.Headers.find("Content-Type"), actual.Request.Headers.end());
EXPECT_EQ(actual.Request.Headers.at("Content-Type"), "application/x-www-form-urlencoded");

EXPECT_NE(actual.Request.Headers.find("Host"), actual.Request.Headers.end());
EXPECT_EQ(actual.Request.Headers.at("Host"), "authority.url");

EXPECT_EQ(actual.Response.Token, "ACCESSTOKEN1");
EXPECT_EQ(actual.Response.ExpiresOn, Azure::DateTime(2021, 1, 1, 1));
}