From f2bd22794d1990c48d066b85c930d33b99fe3465 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 10 Nov 2023 21:00:14 -0800 Subject: [PATCH] Incapsulate all the defaults for options values such as authority host in a helper class and add authority host url validation. (#5155) * Incapsulate all the defaults for options values such as authority host in a helper class and add authority host url validation. * Address PR feedback. --- sdk/core/azure-core/test/ut/url_test.cpp | 3 + sdk/identity/azure-identity/CHANGELOG.md | 2 + .../client_certificate_credential.hpp | 4 +- .../identity/client_secret_credential.hpp | 4 +- .../detail/client_credential_core.hpp | 34 +++++++ .../identity/workload_identity_credential.hpp | 8 +- .../src/client_credential_core.cpp | 15 +-- .../src/workload_identity_credential.cpp | 31 +----- .../ut/client_certificate_credential_test.cpp | 2 +- .../test/ut/client_secret_credential_test.cpp | 2 +- .../ut/workload_identity_credential_test.cpp | 98 ++++++++++++++----- 11 files changed, 135 insertions(+), 68 deletions(-) diff --git a/sdk/core/azure-core/test/ut/url_test.cpp b/sdk/core/azure-core/test/ut/url_test.cpp index bfb576954..64218ecab 100644 --- a/sdk/core/azure-core/test/ut/url_test.cpp +++ b/sdk/core/azure-core/test/ut/url_test.cpp @@ -293,6 +293,9 @@ namespace Azure { namespace Core { namespace Test { { Core::Url url; EXPECT_EQ(url.GetAbsoluteUrl(), std::string()); + + Core::Url empty(""); + EXPECT_EQ(empty.GetAbsoluteUrl(), std::string()); } TEST(URL, AppendPathSlash) diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index e83c94a0e..62e7966e5 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -12,10 +12,12 @@ - Harden checks for the tenant ID. - Disallow space character when validating tenant id and scopes as input for `AzureCliCredential`. +- Add authority host url validation to reject non-HTTPS schemes. ### Other Changes - Create separate lists of characters that are allowed within tenant ids and scopes in `AzureCliCredential`. +- Add default values to some `WorkloadIdentityCredentialOptions` fields such as authority host by reading them from the environment. - Add logging to `WorkloadIdentityCredential` to help with debugging. ## 1.6.0-beta.3 (2023-10-12) 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 eb96feb3e..91568d9a8 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,7 +13,6 @@ #include #include -#include #include #include @@ -53,8 +52,7 @@ namespace Azure { namespace Identity { * clouds' Microsoft Entra authentication endpoints: * https://learn.microsoft.com/azure/active-directory/develop/authentication-national-cloud. */ - std::string AuthorityHost - = Azure::Core::_internal::Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName); + std::string AuthorityHost = _detail::DefaultOptionValues::GetAuthorityHost(); /** * @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 4b7b1a5df..6de87ab01 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,7 +13,6 @@ #include #include -#include #include #include @@ -41,8 +40,7 @@ namespace Azure { namespace Identity { * clouds' Microsoft Entra authentication endpoints: * https://learn.microsoft.com/azure/active-directory/develop/authentication-national-cloud. */ - std::string AuthorityHost - = Azure::Core::_internal::Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName); + std::string AuthorityHost = _detail::DefaultOptionValues::GetAuthorityHost(); /** * @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 9998f546c..cef582422 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 @@ -6,6 +6,7 @@ #include "azure/identity/dll_import_export.hpp" #include +#include #include #include @@ -13,6 +14,39 @@ namespace Azure { namespace Identity { namespace _detail { constexpr auto AzureAuthorityHostEnvVarName = "AZURE_AUTHORITY_HOST"; + constexpr auto AzureTenantIdEnvVarName = "AZURE_TENANT_ID"; + constexpr auto AzureClientIdEnvVarName = "AZURE_CLIENT_ID"; + constexpr auto AzureFederatedTokenFileEnvVarName = "AZURE_FEDERATED_TOKEN_FILE"; + const std::string AadGlobalAuthority = "https://login.microsoftonline.com/"; + + class DefaultOptionValues final { + DefaultOptionValues() = delete; + ~DefaultOptionValues() = delete; + + public: + static std::string GetAuthorityHost() + { + const std::string envAuthHost + = Core::_internal::Environment::GetVariable(AzureAuthorityHostEnvVarName); + + return envAuthHost.empty() ? AadGlobalAuthority : envAuthHost; + } + + static std::string GetTenantId() + { + return Core::_internal::Environment::GetVariable(AzureTenantIdEnvVarName); + } + + static std::string GetClientId() + { + return Core::_internal::Environment::GetVariable(AzureClientIdEnvVarName); + } + + static std::string GetFederatedTokenFile() + { + return Core::_internal::Environment::GetVariable(AzureFederatedTokenFileEnvVarName); + } + }; class ClientCredentialCore final { std::vector m_additionallyAllowedTenants; 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 127380a39..b1f6a644b 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 @@ -31,13 +31,13 @@ namespace Azure { namespace Identity { * @brief The TenantID of the service principal. Defaults to the value of the environment * variable AZURE_TENANT_ID. */ - std::string TenantId; + std::string TenantId = _detail::DefaultOptionValues::GetTenantId(); /** * @brief The ClientID of the service principal. Defaults to the value of the environment * variable AZURE_CLIENT_ID. */ - std::string ClientId; + std::string ClientId = _detail::DefaultOptionValues::GetClientId(); /** * @brief Authentication authority URL. @@ -49,13 +49,13 @@ namespace Azure { namespace Identity { * clouds' Microsoft Entra authentication endpoints: * https://learn.microsoft.com/azure/active-directory/develop/authentication-national-cloud. */ - std::string AuthorityHost; + std::string AuthorityHost = _detail::DefaultOptionValues::GetAuthorityHost(); /** * @brief The path of a file containing a Kubernetes service account token. Defaults to the * value of the environment variable AZURE_FEDERATED_TOKEN_FILE. */ - std::string TokenFilePath; + std::string TokenFilePath = _detail::DefaultOptionValues::GetFederatedTokenFile(); /** * @brief For multi-tenant applications, specifies additional tenants for which the credential diff --git a/sdk/identity/azure-identity/src/client_credential_core.cpp b/sdk/identity/azure-identity/src/client_credential_core.cpp index cc393a78b..81d99a299 100644 --- a/sdk/identity/azure-identity/src/client_credential_core.cpp +++ b/sdk/identity/azure-identity/src/client_credential_core.cpp @@ -13,27 +13,28 @@ using Azure::Core::Credentials::TokenRequestContext; using Azure::Identity::_detail::TenantIdResolver; using Azure::Identity::_detail::TokenCredentialImpl; -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. +// 3. If that environment variable isn't set or 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.empty() ? AadGlobalAuthority : authorityHost)), - m_tenantId(std::move(tenantId)) + m_authorityHost(Url(authorityHost)), m_tenantId(std::move(tenantId)) { } Url ClientCredentialCore::GetRequestUrl(std::string const& tenantId) const { + if (m_authorityHost.GetScheme() != "https") + { + throw Azure::Core::Credentials::AuthenticationException( + "Authority host must be a TLS protected (https) endpoint."); + } + auto requestUrl = m_authorityHost; requestUrl.AppendPath(tenantId); requestUrl.AppendPath(TenantIdResolver::IsAdfs(tenantId) ? "oauth2/token" : "oauth2/v2.0/token"); diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index 65fdab0d6..19607ef12 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -25,12 +25,6 @@ using Azure::Identity::_detail::IdentityLog; using Azure::Identity::_detail::TenantIdResolver; using Azure::Identity::_detail::TokenCredentialImpl; -namespace { -constexpr auto AzureTenantIdEnvVarName = "AZURE_TENANT_ID"; -constexpr auto AzureClientIdEnvVarName = "AZURE_CLIENT_ID"; -constexpr auto AzureFederatedTokenFileEnvVarName = "AZURE_FEDERATED_TOKEN_FILE"; -} // namespace - WorkloadIdentityCredential::WorkloadIdentityCredential( WorkloadIdentityCredentialOptions const& options) : TokenCredential("WorkloadIdentityCredential"), m_clientCredentialCore( @@ -43,23 +37,6 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( std::string authorityHost = options.AuthorityHost; m_tokenFilePath = options.TokenFilePath; - if (tenantId.empty()) - { - tenantId = Environment::GetVariable(AzureTenantIdEnvVarName); - } - if (clientId.empty()) - { - clientId = Environment::GetVariable(AzureClientIdEnvVarName); - } - if (authorityHost.empty()) - { - authorityHost = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName); - } - if (m_tokenFilePath.empty()) - { - m_tokenFilePath = Environment::GetVariable(AzureFederatedTokenFileEnvVarName); - } - if (!tenantId.empty() && !clientId.empty() && !m_tokenFilePath.empty()) { m_clientCredentialCore = Azure::Identity::_detail::ClientCredentialCore( @@ -90,13 +67,13 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( : TokenCredential("WorkloadIdentityCredential"), m_clientCredentialCore("", "", std::vector()) { - std::string tenantId = Environment::GetVariable(AzureTenantIdEnvVarName); - std::string clientId = Environment::GetVariable(AzureClientIdEnvVarName); - m_tokenFilePath = Environment::GetVariable(AzureFederatedTokenFileEnvVarName); + std::string const tenantId = _detail::DefaultOptionValues::GetTenantId(); + std::string const clientId = _detail::DefaultOptionValues::GetClientId(); + m_tokenFilePath = _detail::DefaultOptionValues::GetFederatedTokenFile(); if (!tenantId.empty() && !clientId.empty() && !m_tokenFilePath.empty()) { - std::string authorityHost = Environment::GetVariable(_detail::AzureAuthorityHostEnvVarName); + std::string const authorityHost = _detail::DefaultOptionValues::GetAuthorityHost(); m_clientCredentialCore = Azure::Identity::_detail::ClientCredentialCore( tenantId, authorityHost, std::vector()); 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 e20368f9b..6103da818 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 @@ -155,7 +155,7 @@ TEST(ClientCertificateCredential, GetOptionsFromEnvironment) CredentialTestHelper::EnvironmentOverride const env(envVars); ClientCertificateCredentialOptions options; - EXPECT_EQ(options.AuthorityHost, ""); + EXPECT_EQ(options.AuthorityHost, "https://login.microsoftonline.com/"); } { 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 73f6c9f4a..17a4b8d27 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 @@ -28,7 +28,7 @@ TEST(ClientSecretCredential, GetOptionsFromEnvironment) CredentialTestHelper::EnvironmentOverride const env(envVars); ClientSecretCredentialOptions options; - EXPECT_EQ(options.AuthorityHost, ""); + EXPECT_EQ(options.AuthorityHost, "https://login.microsoftonline.com/"); } { diff --git a/sdk/identity/azure-identity/test/ut/workload_identity_credential_test.cpp b/sdk/identity/azure-identity/test/ut/workload_identity_credential_test.cpp index 96db871b5..64106f59f 100644 --- a/sdk/identity/azure-identity/test/ut/workload_identity_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/workload_identity_credential_test.cpp @@ -40,36 +40,90 @@ TEST(WorkloadIdentityCredential, GetCredentialName) TEST(WorkloadIdentityCredential, GetOptionsFromEnvironment) { - CredentialTestHelper::EnvironmentOverride const env( - {{"AZURE_TENANT_ID", "01234567-89ab-cdef-fedc-ba8976543210"}, - {"AZURE_CLIENT_ID", "fedcba98-7654-3210-0123-456789abcdef"}, - {"AZURE_AUTHORITY_HOST", ""}, - {"AZURE_FEDERATED_TOKEN_FILE", TempCertFile::Path}}); + { + CredentialTestHelper::EnvironmentOverride const env( + {{"AZURE_TENANT_ID", "01234567-89ab-cdef-fedc-ba8976543210"}, + {"AZURE_CLIENT_ID", "fedcba98-7654-3210-0123-456789abcdef"}, + {"AZURE_AUTHORITY_HOST", ""}, + {"AZURE_FEDERATED_TOKEN_FILE", TempCertFile::Path}}); - WorkloadIdentityCredential const credDefault; - EXPECT_EQ(credDefault.GetCredentialName(), "WorkloadIdentityCredential"); + WorkloadIdentityCredential const credDefault; + EXPECT_EQ(credDefault.GetCredentialName(), "WorkloadIdentityCredential"); - WorkloadIdentityCredentialOptions options; - WorkloadIdentityCredential const cred(options); - EXPECT_EQ(cred.GetCredentialName(), "WorkloadIdentityCredential"); + WorkloadIdentityCredentialOptions options; + WorkloadIdentityCredential const cred(options); + EXPECT_EQ(cred.GetCredentialName(), "WorkloadIdentityCredential"); + + EXPECT_EQ(options.TenantId, "01234567-89ab-cdef-fedc-ba8976543210"); + EXPECT_EQ(options.ClientId, "fedcba98-7654-3210-0123-456789abcdef"); + EXPECT_EQ(options.AuthorityHost, "https://login.microsoftonline.com/"); + EXPECT_EQ(options.TokenFilePath, TempCertFile::Path); + } + + { + std::map envVars = {{"AZURE_AUTHORITY_HOST", "foo"}}; + CredentialTestHelper::EnvironmentOverride const env(envVars); + + WorkloadIdentityCredentialOptions options; + options.AuthorityHost = "bar"; + EXPECT_EQ(options.AuthorityHost, "bar"); + } + + { + std::map envVars + = {{"AZURE_AUTHORITY_HOST", "https://microsoft.com/"}}; + CredentialTestHelper::EnvironmentOverride const env(envVars); + + WorkloadIdentityCredentialOptions options; + EXPECT_EQ(options.AuthorityHost, "https://microsoft.com/"); + } } TEST(WorkloadIdentityCredential, GetOptionsFromEnvironmentInvalid) { - CredentialTestHelper::EnvironmentOverride const env( - {{"AZURE_TENANT_ID", ""}, - {"AZURE_CLIENT_ID", ""}, - {"AZURE_AUTHORITY_HOST", ""}, - {"AZURE_FEDERATED_TOKEN_FILE", ""}}); + { + CredentialTestHelper::EnvironmentOverride const env( + {{"AZURE_TENANT_ID", ""}, + {"AZURE_CLIENT_ID", ""}, + {"AZURE_AUTHORITY_HOST", ""}, + {"AZURE_FEDERATED_TOKEN_FILE", ""}}); - TokenRequestContext trc; - trc.Scopes.push_back("https://storage.azure.com/.default"); + TokenRequestContext trc; + trc.Scopes.push_back("https://storage.azure.com/.default"); - WorkloadIdentityCredential const credDefault; - EXPECT_THROW(credDefault.GetToken(trc, {}), AuthenticationException); - WorkloadIdentityCredentialOptions options; - WorkloadIdentityCredential const cred(options); - EXPECT_THROW(cred.GetToken(trc, {}), AuthenticationException); + WorkloadIdentityCredential const credDefault; + EXPECT_THROW(credDefault.GetToken(trc, {}), AuthenticationException); + WorkloadIdentityCredentialOptions options; + WorkloadIdentityCredential const cred(options); + EXPECT_THROW(cred.GetToken(trc, {}), AuthenticationException); + } + + // The http scheme is not supported. + { + CredentialTestHelper::EnvironmentOverride const env( + {{"AZURE_TENANT_ID", "01234567-89ab-cdef-fedc-ba8976543210"}, + {"AZURE_CLIENT_ID", "fedcba98-7654-3210-0123-456789abcdef"}, + {"AZURE_AUTHORITY_HOST", "http://microsoft.com/"}, + {"AZURE_FEDERATED_TOKEN_FILE", TempCertFile::Path}}); + + TokenRequestContext trc; + trc.Scopes.push_back("https://storage.azure.com/.default"); + + WorkloadIdentityCredential const credDefault; + EXPECT_THROW(credDefault.GetToken(trc, {}), AuthenticationException); + WorkloadIdentityCredentialOptions options; + WorkloadIdentityCredential const cred(options); + EXPECT_THROW(cred.GetToken(trc, {}), AuthenticationException); + + try + { + auto const token = cred.GetToken(trc, {}); + } + catch (AuthenticationException const& e) + { + EXPECT_TRUE(std::string(e.what()).find("https") != std::string::npos) << e.what(); + } + } } TEST(WorkloadIdentityCredential, Regular)