From 641e9694b2b3db219fd7b12aad7412f3a6ece6f8 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 20 Sep 2023 19:12:58 -0700 Subject: [PATCH 1/4] Change the default option for authority host to be read from the environment first. --- .gitignore | 1 + .../identity/client_certificate_credential.hpp | 4 +++- .../azure/identity/client_secret_credential.hpp | 4 +++- .../identity/detail/client_credential_core.hpp | 4 ++-- .../azure-identity/src/client_credential_core.cpp | 15 ++++++++++----- .../azure-identity/src/environment_credential.cpp | 3 +-- .../src/workload_identity_credential.cpp | 13 ++----------- 7 files changed, 22 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index 19149e0842..c53048564d 100644 --- a/.gitignore +++ b/.gitignore @@ -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/** diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp index 320434a9d6..59a9509041 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -50,7 +51,8 @@ namespace Azure { namespace Identity { * 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 diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp index fe60e8a27c..5c69c4d550 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -38,7 +39,8 @@ namespace Azure { namespace Identity { * 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 diff --git a/sdk/identity/azure-identity/inc/azure/identity/detail/client_credential_core.hpp b/sdk/identity/azure-identity/inc/azure/identity/detail/client_credential_core.hpp index eec920078f..9998f546c4 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/detail/client_credential_core.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/detail/client_credential_core.hpp @@ -12,14 +12,14 @@ #include namespace Azure { namespace Identity { namespace _detail { + constexpr auto AzureAuthorityHostEnvVarName = "AZURE_AUTHORITY_HOST"; + class ClientCredentialCore final { std::vector 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, diff --git a/sdk/identity/azure-identity/src/client_credential_core.cpp b/sdk/identity/azure-identity/src/client_credential_core.cpp index 8ade161d83..8feaf924c0 100644 --- a/sdk/identity/azure-identity/src/client_credential_core.cpp +++ b/sdk/identity/azure-identity/src/client_credential_core.cpp @@ -6,8 +6,6 @@ #include "private/tenant_id_resolver.hpp" #include "private/token_credential_impl.hpp" -#include - using Azure::Identity::_detail::ClientCredentialCore; using Azure::Core::Url; @@ -15,15 +13,22 @@ 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 be 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 additionallyAllowedTenants) : m_additionallyAllowedTenants(std::move(additionallyAllowedTenants)), - m_authorityHost(Url(authorityHost)), m_tenantId(std::move(tenantId)) + m_authorityHost(Url(authorityHost.empty() ? AadGlobalAuthority : authorityHost)), + m_tenantId(std::move(tenantId)) { } diff --git a/sdk/identity/azure-identity/src/environment_credential.cpp b/sdk/identity/azure-identity/src/environment_credential.cpp index ef8655ec90..e4c5654045 100644 --- a/sdk/identity/azure-identity/src/environment_credential.cpp +++ b/sdk/identity/azure-identity/src/environment_credential.cpp @@ -28,7 +28,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( @@ -46,7 +45,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); diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index 14f90f84a9..cd1dfc2813 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -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 @@ -52,11 +51,7 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( } if (authorityHost.empty()) { - authorityHost = Environment::GetVariable(AzureAuthorityHostEnvVarName); - if (authorityHost.empty()) - { - authorityHost = _detail::ClientCredentialCore::AadGlobalAuthority; - } + authorityHost = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName); } if (m_tokenFilePath.empty()) { @@ -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()); From 5a95b62230520d6c166860d8ca1d18b940619dfa Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 20 Sep 2023 19:15:13 -0700 Subject: [PATCH 2/4] Update changelog. --- sdk/identity/azure-identity/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index 80eff317da..64b4138d7e 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Change the default value for the authority host option to be read from the environment variable first. + ### Other Changes ## 1.6.0-beta.2 (2023-09-13) From 4a03f5002617823be34fc8aeb79e8dbe0afe1825 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 3 Oct 2023 18:45:16 -0700 Subject: [PATCH 3/4] Update doc comment and refer to the env var correctly. --- .../inc/azure/identity/client_certificate_credential.hpp | 3 ++- .../inc/azure/identity/client_secret_credential.hpp | 3 ++- .../azure-identity/src/environment_credential.cpp | 9 +++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp index 59a9509041..73befb5677 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp @@ -45,7 +45,8 @@ namespace Azure { namespace Identity { { /** * @brief Authentication authority URL. - * @note Default value is Azure AD global authority (https://login.microsoftonline.com/). + * @note Default value is read from the 'AZURE_AUTHORITY_HOST' environment variable with Azure + * AD global authority as the fallback (https://login.microsoftonline.com/). * * @note Example of an authority host string: "https://login.microsoftonline.us/". See national * clouds' Azure AD authentication endpoints: diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp index 5c69c4d550..5d415a6ac2 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp @@ -33,7 +33,8 @@ namespace Azure { namespace Identity { { /** * @brief Authentication authority URL. - * @note Default value is Azure AD global authority (https://login.microsoftonline.com/). + * @note Default value is read from the 'AZURE_AUTHORITY_HOST' environment variable with Azure + * AD global authority as the fallback (https://login.microsoftonline.com/). * * @note Example of an authority host string: "https://login.microsoftonline.us/". See national * clouds' Azure AD authentication endpoints: diff --git a/sdk/identity/azure-identity/src/environment_credential.cpp b/sdk/identity/azure-identity/src/environment_credential.cpp index e4c5654045..8074093de4 100644 --- a/sdk/identity/azure-identity/src/environment_credential.cpp +++ b/sdk/identity/azure-identity/src/environment_credential.cpp @@ -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 @@ -64,7 +65,7 @@ EnvironmentCredential::EnvironmentCredential( if (!authority.empty()) { - envVarsToParams.push_back({AzureAuthorityHostEnvVarName, "authorityHost"}); + envVarsToParams.push_back({_detail::AzureAuthorityHostEnvVarName, "authorityHost"}); clientSecretCredentialOptions.AuthorityHost = authority; } @@ -84,7 +85,7 @@ EnvironmentCredential::EnvironmentCredential( if (!authority.empty()) { - envVarsToParams.push_back({AzureAuthorityHostEnvVarName, "authorityHost"}); + envVarsToParams.push_back({_detail::AzureAuthorityHostEnvVarName, "authorityHost"}); clientCertificateCredentialOptions.AuthorityHost = authority; } @@ -108,7 +109,7 @@ 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 envVarStatus[] = { @@ -116,7 +117,7 @@ EnvironmentCredential::EnvironmentCredential( {AzureClientIdEnvVarName, !clientId.empty()}, {AzureClientSecretEnvVarName, !clientSecret.empty()}, {AzureClientCertificatePathEnvVarName, !clientCertificatePath.empty()}, - {AzureAuthorityHostEnvVarName, !authority.empty()}, + {_detail::AzureAuthorityHostEnvVarName, !authority.empty()}, }; for (auto const& status : envVarStatus) { From 72c72194f2230b989a95c07a12b94fd069727979 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 4 Oct 2023 18:50:38 -0700 Subject: [PATCH 4/4] Update doc comments and add unit tests. --- .../client_certificate_credential.hpp | 4 ++-- .../identity/client_secret_credential.hpp | 4 ++-- .../identity/workload_identity_credential.hpp | 2 +- .../src/client_credential_core.cpp | 2 +- .../ut/client_certificate_credential_test.cpp | 20 +++++++++++++++++++ .../test/ut/client_secret_credential_test.cpp | 20 +++++++++++++++++++ 6 files changed, 46 insertions(+), 6 deletions(-) diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp index 73befb5677..a66e185ac3 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp @@ -45,8 +45,8 @@ namespace Azure { namespace Identity { { /** * @brief Authentication authority URL. - * @note Default value is read from the 'AZURE_AUTHORITY_HOST' environment variable with Azure - * AD global authority as the fallback (https://login.microsoftonline.com/). + * @note Defaults to the value of the environment variable 'AZURE_AUTHORITY_HOST'. If that's not + * set, the default value is Azure AD global authority (https://login.microsoftonline.com/). * * @note Example of an authority host string: "https://login.microsoftonline.us/". See national * clouds' Azure AD authentication endpoints: diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp index 5d415a6ac2..2a5bffc375 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp @@ -33,8 +33,8 @@ namespace Azure { namespace Identity { { /** * @brief Authentication authority URL. - * @note Default value is read from the 'AZURE_AUTHORITY_HOST' environment variable with Azure - * AD global authority as the fallback (https://login.microsoftonline.com/). + * @note Defaults to the value of the environment variable 'AZURE_AUTHORITY_HOST'. If that's not + * set, the default value is Azure AD global authority (https://login.microsoftonline.com/). * * @note Example of an authority host string: "https://login.microsoftonline.us/". See national * clouds' Azure AD authentication endpoints: diff --git a/sdk/identity/azure-identity/inc/azure/identity/workload_identity_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/workload_identity_credential.hpp index 2dc3613f6f..a4f1947b15 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/workload_identity_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/workload_identity_credential.hpp @@ -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 * set, the default value is Azure AD global authority (https://login.microsoftonline.com/). * * @note Example of an authority host string: "https://login.microsoftonline.us/". See national diff --git a/sdk/identity/azure-identity/src/client_credential_core.cpp b/sdk/identity/azure-identity/src/client_credential_core.cpp index 8feaf924c0..cc393a78b4 100644 --- a/sdk/identity/azure-identity/src/client_credential_core.cpp +++ b/sdk/identity/azure-identity/src/client_credential_core.cpp @@ -17,7 +17,7 @@ namespace { const std::string AadGlobalAuthority = "https://login.microsoftonline.com/"; } -// The authority host used be the credentials is in the following order of precedence: +// 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. diff --git a/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp b/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp index 9a2d1b942b..e20368f9b4 100644 --- a/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp @@ -148,6 +148,26 @@ TEST_P(GetCredentialName, ) EXPECT_EQ(cred.GetCredentialName(), "ClientCertificateCredential"); } +TEST(ClientCertificateCredential, GetOptionsFromEnvironment) +{ + { + std::map envVars = {{"AZURE_AUTHORITY_HOST", ""}}; + CredentialTestHelper::EnvironmentOverride const env(envVars); + + ClientCertificateCredentialOptions options; + EXPECT_EQ(options.AuthorityHost, ""); + } + + { + std::map 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( diff --git a/sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp b/sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp index 7e79f38dc1..73f6c9f4ab 100644 --- a/sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp @@ -21,6 +21,26 @@ TEST(ClientSecretCredential, GetCredentialName) EXPECT_EQ(cred.GetCredentialName(), "ClientSecretCredential"); } +TEST(ClientSecretCredential, GetOptionsFromEnvironment) +{ + { + std::map envVars = {{"AZURE_AUTHORITY_HOST", ""}}; + CredentialTestHelper::EnvironmentOverride const env(envVars); + + ClientSecretCredentialOptions options; + EXPECT_EQ(options.AuthorityHost, ""); + } + + { + std::map envVars + = {{"AZURE_AUTHORITY_HOST", "https://microsoft.com/"}}; + CredentialTestHelper::EnvironmentOverride const env(envVars); + + ClientSecretCredentialOptions options; + EXPECT_EQ(options.AuthorityHost, "https://microsoft.com/"); + } +} + TEST(ClientSecretCredential, Regular) { auto const actual = CredentialTestHelper::SimulateTokenRequest(