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

Change the default value for the authority host option to be read from the environment variable first. #4980

Merged
merged 4 commits into from
Oct 5, 2023
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ bld*/
[Oo]bj/
[Ll]og/
[Oo]ut/
[Tt]emp
#these are for build from source temp directories
[Ss]dk/**/[Ss]dk/
**/[Oo]ut/**
Expand Down
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

- Change the default value for the authority host option to be read from the environment variable first.
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved

### Other Changes

## 1.6.0-beta.2 (2023-09-13)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <azure/core/credentials/credentials.hpp>
#include <azure/core/credentials/token_credential_options.hpp>
#include <azure/core/internal/environment.hpp>
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
#include <azure/core/internal/unique_handle.hpp>
#include <azure/core/url.hpp>

Expand Down Expand Up @@ -44,13 +45,15 @@ namespace Azure { namespace Identity {
{
/**
* @brief Authentication authority URL.
* @note Default value is Azure AD global authority (https://login.microsoftonline.com/).
* @note Defaults to the value of the environment variable 'AZURE_AUTHORITY_HOST'. If that's not
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
* set, the default value is Azure AD global authority (https://login.microsoftonline.com/).
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
*
* @note Example of an authority host string: "https://login.microsoftonline.us/". See national
* clouds' Azure AD authentication endpoints:
* https://docs.microsoft.com/azure/active-directory/develop/authentication-national-cloud.
*/
std::string AuthorityHost = _detail::ClientCredentialCore::AadGlobalAuthority;
std::string AuthorityHost
= Azure::Core::_internal::Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName);
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved

/**
* @brief For multi-tenant applications, specifies additional tenants for which the credential
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <azure/core/credentials/credentials.hpp>
#include <azure/core/credentials/token_credential_options.hpp>
#include <azure/core/internal/environment.hpp>
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
#include <azure/core/url.hpp>

#include <memory>
Expand All @@ -32,13 +33,15 @@ namespace Azure { namespace Identity {
{
/**
* @brief Authentication authority URL.
* @note Default value is Azure AD global authority (https://login.microsoftonline.com/).
* @note Defaults to the value of the environment variable 'AZURE_AUTHORITY_HOST'. If that's not
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
* set, the default value is Azure AD global authority (https://login.microsoftonline.com/).
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
*
* @note Example of an authority host string: "https://login.microsoftonline.us/". See national
* clouds' Azure AD authentication endpoints:
* https://docs.microsoft.com/azure/active-directory/develop/authentication-national-cloud.
*/
std::string AuthorityHost = _detail::ClientCredentialCore::AadGlobalAuthority;
std::string AuthorityHost
= Azure::Core::_internal::Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName);

/**
* @brief For multi-tenant applications, specifies additional tenants for which the credential
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
#include <vector>

namespace Azure { namespace Identity { namespace _detail {
constexpr auto AzureAuthorityHostEnvVarName = "AZURE_AUTHORITY_HOST";

ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
class ClientCredentialCore final {
std::vector<std::string> m_additionallyAllowedTenants;
Core::Url m_authorityHost;
std::string m_tenantId;

public:
AZ_IDENTITY_DLLEXPORT static std::string const AadGlobalAuthority;

explicit ClientCredentialCore(
std::string tenantId,
std::string const& authorityHost,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace Azure { namespace Identity {

/**
* @brief Authentication authority URL.
* @note Defaults to the value of the environment variable AZURE_AUTHORITY_HOST. If that's not
* @note Defaults to the value of the environment variable 'AZURE_AUTHORITY_HOST'. If that's not
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
* set, the default value is Azure AD global authority (https://login.microsoftonline.com/).
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
*
* @note Example of an authority host string: "https://login.microsoftonline.us/". See national
Expand Down
15 changes: 10 additions & 5 deletions sdk/identity/azure-identity/src/client_credential_core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,29 @@
#include "private/tenant_id_resolver.hpp"
#include "private/token_credential_impl.hpp"

#include <utility>

using Azure::Identity::_detail::ClientCredentialCore;

using Azure::Core::Url;
using Azure::Core::Credentials::TokenRequestContext;
using Azure::Identity::_detail::TenantIdResolver;
using Azure::Identity::_detail::TokenCredentialImpl;

decltype(ClientCredentialCore::AadGlobalAuthority) ClientCredentialCore::AadGlobalAuthority
= "https://login.microsoftonline.com/";
namespace {
const std::string AadGlobalAuthority = "https://login.microsoftonline.com/";
}

// The authority host used by the credentials is in the following order of precedence:
// 1. AuthorityHost option set/overriden by the user.
// 2. The value of AZURE_AUTHORITY_HOST environment variable, which is the default value of the
// option.
// 3. If the option is empty, use Azure Public Cloud.
ClientCredentialCore::ClientCredentialCore(
std::string tenantId,
std::string const& authorityHost,
std::vector<std::string> additionallyAllowedTenants)
: m_additionallyAllowedTenants(std::move(additionallyAllowedTenants)),
m_authorityHost(Url(authorityHost)), m_tenantId(std::move(tenantId))
m_authorityHost(Url(authorityHost.empty() ? AadGlobalAuthority : authorityHost)),
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
m_tenantId(std::move(tenantId))
{
}

Expand Down
12 changes: 6 additions & 6 deletions sdk/identity/azure-identity/src/environment_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "azure/identity/client_certificate_credential.hpp"
#include "azure/identity/client_secret_credential.hpp"
#include "azure/identity/detail/client_credential_core.hpp"
#include "private/identity_log.hpp"

#include <azure/core/azure_assert.hpp>
Expand All @@ -28,7 +29,6 @@ namespace {
constexpr auto AzureTenantIdEnvVarName = "AZURE_TENANT_ID";
constexpr auto AzureClientIdEnvVarName = "AZURE_CLIENT_ID";
constexpr auto AzureClientSecretEnvVarName = "AZURE_CLIENT_SECRET";
constexpr auto AzureAuthorityHostEnvVarName = "AZURE_AUTHORITY_HOST";
constexpr auto AzureClientCertificatePathEnvVarName = "AZURE_CLIENT_CERTIFICATE_PATH";

void PrintCredentialCreationLogMessage(
Expand All @@ -46,7 +46,7 @@ EnvironmentCredential::EnvironmentCredential(
auto clientId = Environment::GetVariable(AzureClientIdEnvVarName);

auto clientSecret = Environment::GetVariable(AzureClientSecretEnvVarName);
auto authority = Environment::GetVariable(AzureAuthorityHostEnvVarName);
auto authority = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName);

auto clientCertificatePath = Environment::GetVariable(AzureClientCertificatePathEnvVarName);

Expand All @@ -65,7 +65,7 @@ EnvironmentCredential::EnvironmentCredential(

if (!authority.empty())
{
envVarsToParams.push_back({AzureAuthorityHostEnvVarName, "authorityHost"});
envVarsToParams.push_back({_detail::AzureAuthorityHostEnvVarName, "authorityHost"});
clientSecretCredentialOptions.AuthorityHost = authority;
}

Expand All @@ -85,7 +85,7 @@ EnvironmentCredential::EnvironmentCredential(

if (!authority.empty())
{
envVarsToParams.push_back({AzureAuthorityHostEnvVarName, "authorityHost"});
envVarsToParams.push_back({_detail::AzureAuthorityHostEnvVarName, "authorityHost"});
clientCertificateCredentialOptions.AuthorityHost = authority;
}

Expand All @@ -109,15 +109,15 @@ EnvironmentCredential::EnvironmentCredential(
auto logMsg = GetCredentialName() + ": Both '" + AzureTenantIdEnvVarName + "' and '"
+ AzureClientIdEnvVarName + "', and at least one of '" + AzureClientSecretEnvVarName
+ "', '" + AzureClientCertificatePathEnvVarName + "' needs to be set. Additionally, '"
+ AzureAuthorityHostEnvVarName
+ _detail::AzureAuthorityHostEnvVarName
+ "' could be set to override the default authority host. Currently:\n";

std::pair<char const*, bool> envVarStatus[] = {
{AzureTenantIdEnvVarName, !tenantId.empty()},
{AzureClientIdEnvVarName, !clientId.empty()},
{AzureClientSecretEnvVarName, !clientSecret.empty()},
{AzureClientCertificatePathEnvVarName, !clientCertificatePath.empty()},
{AzureAuthorityHostEnvVarName, !authority.empty()},
{_detail::AzureAuthorityHostEnvVarName, !authority.empty()},
};
for (auto const& status : envVarStatus)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ using Azure::Identity::_detail::TokenCredentialImpl;
namespace {
constexpr auto AzureTenantIdEnvVarName = "AZURE_TENANT_ID";
constexpr auto AzureClientIdEnvVarName = "AZURE_CLIENT_ID";
constexpr auto AzureAuthorityHostEnvVarName = "AZURE_AUTHORITY_HOST";
constexpr auto AzureFederatedTokenFileEnvVarName = "AZURE_FEDERATED_TOKEN_FILE";
} // namespace

Expand All @@ -52,11 +51,7 @@ WorkloadIdentityCredential::WorkloadIdentityCredential(
}
if (authorityHost.empty())
{
authorityHost = Environment::GetVariable(AzureAuthorityHostEnvVarName);
if (authorityHost.empty())
{
authorityHost = _detail::ClientCredentialCore::AadGlobalAuthority;
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
}
authorityHost = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName);
}
if (m_tokenFilePath.empty())
{
Expand Down Expand Up @@ -89,11 +84,7 @@ WorkloadIdentityCredential::WorkloadIdentityCredential(

if (!tenantId.empty() && !clientId.empty() && !m_tokenFilePath.empty())
{
std::string authorityHost = Environment::GetVariable(AzureAuthorityHostEnvVarName);
if (authorityHost.empty())
{
authorityHost = _detail::ClientCredentialCore::AadGlobalAuthority;
}
std::string authorityHost = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName);

m_clientCredentialCore = Azure::Identity::_detail::ClientCredentialCore(
tenantId, authorityHost, std::vector<std::string>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,26 @@ TEST_P(GetCredentialName, )
EXPECT_EQ(cred.GetCredentialName(), "ClientCertificateCredential");
}

TEST(ClientCertificateCredential, GetOptionsFromEnvironment)
{
{
std::map<std::string, std::string> envVars = {{"AZURE_AUTHORITY_HOST", ""}};
CredentialTestHelper::EnvironmentOverride const env(envVars);

ClientCertificateCredentialOptions options;
EXPECT_EQ(options.AuthorityHost, "");
}

{
std::map<std::string, std::string> envVars
= {{"AZURE_AUTHORITY_HOST", "https://microsoft.com/"}};
CredentialTestHelper::EnvironmentOverride const env(envVars);

ClientCertificateCredentialOptions options;
EXPECT_EQ(options.AuthorityHost, "https://microsoft.com/");
}
}

TEST_P(GetToken, )
{
auto const actual = CredentialTestHelper::SimulateTokenRequest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,26 @@ TEST(ClientSecretCredential, GetCredentialName)
EXPECT_EQ(cred.GetCredentialName(), "ClientSecretCredential");
}

TEST(ClientSecretCredential, GetOptionsFromEnvironment)
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
{
{
std::map<std::string, std::string> envVars = {{"AZURE_AUTHORITY_HOST", ""}};
CredentialTestHelper::EnvironmentOverride const env(envVars);

ClientSecretCredentialOptions options;
EXPECT_EQ(options.AuthorityHost, "");
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
}

{
std::map<std::string, std::string> envVars
= {{"AZURE_AUTHORITY_HOST", "https://microsoft.com/"}};
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
CredentialTestHelper::EnvironmentOverride const env(envVars);

ClientSecretCredentialOptions options;
EXPECT_EQ(options.AuthorityHost, "https://microsoft.com/");
}
}

TEST(ClientSecretCredential, Regular)
{
auto const actual = CredentialTestHelper::SimulateTokenRequest(
Expand Down