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 all 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
1 change: 1 addition & 0 deletions sdk/core/azure-core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ set(
${CURL_TRANSPORT_ADAPTER_INC}
${WIN_TRANSPORT_ADAPTER_INC}
inc/azure/core/credentials/credentials.hpp
inc/azure/core/credentials/token_credential_options.hpp
Copy link
Member

Choose a reason for hiding this comment

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

We need to include these headers to core.hpp and add tests to the simplified header.
Fixed in #2070

inc/azure/core/cryptography/hash.hpp
inc/azure/core/diagnostics/logger.hpp
inc/azure/core/http/http.hpp
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// SPDX-License-Identifier: MIT

/**
* @file
* @brief Options for #Azure::Core::Credentials::TokenCredential.
*/

#pragma once

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

namespace Azure { namespace Core { namespace Credentials {
/**
* @brief Defines options for #Azure::Core::Credentials::TokenCredential.
*/
struct TokenCredentialOptions : public Azure::Core::_internal::ClientOptions
Copy link
Member

Choose a reason for hiding this comment

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

Needs CL entry.

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 not a breaking change, and has the default value. All the code you could compile before can still compile now.

Copy link
Member

@ahsonkhan ahsonkhan Mar 30, 2021

Choose a reason for hiding this comment

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

I understand. It's a new type/new feature (which are also added to CL) :)

{
};
}}} // namespace Azure::Core::Credentials
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
#include "azure/identity/dll_import_export.hpp"

#include <azure/core/credentials/credentials.hpp>
#include <azure/core/credentials/token_credential_options.hpp>
#include <azure/core/http/policies/policy.hpp>
#include <azure/core/internal/client_options.hpp>

#include <string>
#include <utility>
Expand All @@ -25,7 +25,7 @@ namespace Azure { namespace Identity {
/**
* @brief Defines options for token authentication.
*/
struct ClientSecretCredentialOptions : public Azure::Core::_internal::ClientOptions
struct ClientSecretCredentialOptions : public Azure::Core::Credentials::TokenCredentialOptions
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
{
public:
/**
Expand Down Expand Up @@ -64,12 +64,32 @@ namespace Azure { namespace Identity {
std::string tenantId,
std::string clientId,
std::string clientSecret,
ClientSecretCredentialOptions options = ClientSecretCredentialOptions())
ClientSecretCredentialOptions options)
: m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)),
m_clientSecret(std::move(clientSecret)), m_options(std::move(options))
{
}

/**
* @brief Construct a Client Secret credential.
Copy link
Member

Choose a reason for hiding this comment

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

This is an odd casing choice. Was it intentional?

*
* @param tenantId Tenant ID.
* @param clientId Client ID.
* @param clientSecret Client Secret.
* @param options #Azure::Core::Credentials::TokenCredentialOptions.
*/
explicit ClientSecretCredential(
std::string tenantId,
std::string clientId,
std::string clientSecret,
Azure::Core::Credentials::TokenCredentialOptions const& options
= Azure::Core::Credentials::TokenCredentialOptions())
: m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)),
m_clientSecret(std::move(clientSecret))
{
static_cast<Azure::Core::Credentials::TokenCredentialOptions&>(m_options) = options;
Copy link
Member

Choose a reason for hiding this comment

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

nit: Add a comment explaining why this cast is OK.

}

Core::Credentials::AccessToken GetToken(
Core::Credentials::TokenRequestContext const& tokenRequestContext,
Core::Context const& context) const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@
#pragma once

#include <azure/core/credentials/credentials.hpp>
#include <azure/core/credentials/token_credential_options.hpp>

#include <memory>

namespace Azure { namespace Identity {

/**
* @brief An environment credential.
*/
Expand All @@ -32,7 +32,9 @@ namespace Azure { namespace Identity {
* - AZURE_USERNAME
* - AZURE_PASSWORD
*/
explicit EnvironmentCredential();
explicit EnvironmentCredential(
Azure::Core::Credentials::TokenCredentialOptions options
= Azure::Core::Credentials::TokenCredentialOptions());

Core::Credentials::AccessToken GetToken(
Core::Credentials::TokenRequestContext const& tokenRequestContext,
Expand Down
54 changes: 45 additions & 9 deletions sdk/identity/azure-identity/src/client_secret_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,40 @@
#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 +61,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 +89,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
30 changes: 16 additions & 14 deletions sdk/identity/azure-identity/src/environment_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@

using namespace Azure::Identity;

EnvironmentCredential::EnvironmentCredential()
EnvironmentCredential::EnvironmentCredential(
Azure::Core::Credentials::TokenCredentialOptions options)
{
#if !defined(WINAPI_PARTITION_DESKTOP) \
|| WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation.
Expand Down Expand Up @@ -53,28 +54,29 @@ EnvironmentCredential::EnvironmentCredential()
{
if (authority != nullptr)
{
ClientSecretCredentialOptions options;
options.AuthorityHost = authority;
ClientSecretCredentialOptions clientSecretCredentialOptions;
static_cast<Core::_internal::ClientOptions&>(clientSecretCredentialOptions) = options;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be TokenCredentialOptions?

Suggested change
static_cast<Core::_internal::ClientOptions&>(clientSecretCredentialOptions) = options;
static_cast<Core::Credentials::TokenCredentialOptions&>(clientSecretCredentialOptions) = options;

What's this line doing? Is it only copying over the fields into the local clientSecretCredentialOptions that come from the base type options? Does it leave the other fields alone?

Also, add comment why static cast is OK here.

Copy link
Member

@ahsonkhan ahsonkhan Mar 30, 2021

Choose a reason for hiding this comment

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

@antkmsft does this need to be fixed for correctness?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now we are fine, but yes, it needs fixing - I'll grab this plus a few other comments and create a follow-up PR.

clientSecretCredentialOptions.AuthorityHost = authority;

m_credentialImpl.reset(
new ClientSecretCredential(tenantId, clientId, clientSecret, options));
m_credentialImpl.reset(new ClientSecretCredential(
tenantId, clientId, clientSecret, clientSecretCredentialOptions));
}
else
{
m_credentialImpl.reset(new ClientSecretCredential(tenantId, clientId, clientSecret));
m_credentialImpl.reset(
new ClientSecretCredential(tenantId, clientId, clientSecret, options));
}
}
// TODO: These credential types are not implemented. Uncomment when implemented.
Copy link
Member

@ahsonkhan ahsonkhan Mar 30, 2021

Choose a reason for hiding this comment

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

Since you are hereand modifying this code already, remove this TODO comment from source and file an issue.

// else if (username != nullptr && password != nullptr)
//{
// m_credentialImpl.reset(
// new UsernamePasswordCredential(username, password, tenantId, clientId));
//}
// {
// m_credentialImpl.reset(
// new UsernamePasswordCredential(tenantId, clientId, username, password, options));
// }
// else if (clientCertificatePath != nullptr)
//{
// m_credentialImpl.reset(
// new ClientCertificateCredential(tenantId, clientId, clientCertificatePath));
//}
// {
// m_credentialImpl.reset(new ClientCertificateCredential(tenantId, clientId, options));
// }
}
#endif
}
Expand Down
3 changes: 3 additions & 0 deletions sdk/identity/azure-identity/test/ut/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@ include(GoogleTest)

add_executable (
azure-identity-test
client_secret_credential.cpp
environment_credential.cpp
macro_guard.cpp
main.cpp
simplified_header.cpp
test_transport.hpp
)

if (MSVC)
Expand Down
Loading