Incapsulate all the defaults for options values such as authority hos…

…t 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

@@ -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)

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)

sdk/identity/azure-identity/inc/azure/identity/client_certificate_credential.hpp

@@ -13,7 +13,6 @@
 
 #include <azure/core/credentials/credentials.hpp>
 #include <azure/core/credentials/token_credential_options.hpp>
-#include <azure/core/internal/environment.hpp>
 #include <azure/core/internal/unique_handle.hpp>
 #include <azure/core/url.hpp>
 
@@ -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

sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp

@@ -13,7 +13,6 @@
 
 #include <azure/core/credentials/credentials.hpp>
 #include <azure/core/credentials/token_credential_options.hpp>
-#include <azure/core/internal/environment.hpp>
 #include <azure/core/url.hpp>
 
 #include <memory>
@@ -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

sdk/identity/azure-identity/inc/azure/identity/detail/client_credential_core.hpp

@@ -6,13 +6,47 @@
 #include "azure/identity/dll_import_export.hpp"
 
 #include <azure/core/credentials/credentials.hpp>
+#include <azure/core/internal/environment.hpp>
 #include <azure/core/url.hpp>
 
 #include <string>
 #include <vector>
 
 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<std::string> m_additionallyAllowedTenants;

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

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<std::string> 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");

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>())
 {
-  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<std::string>());

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/");
   }
 
   {

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/");
   }
 
   {

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");
+
+    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);
+  }
 
-  WorkloadIdentityCredential const credDefault;
-  EXPECT_EQ(credDefault.GetCredentialName(), "WorkloadIdentityCredential");
+  {
+    std::map<std::string, std::string> envVars = {{"AZURE_AUTHORITY_HOST", "foo"}};
+    CredentialTestHelper::EnvironmentOverride const env(envVars);
 
-  WorkloadIdentityCredentialOptions options;
-  WorkloadIdentityCredential const cred(options);
-  EXPECT_EQ(cred.GetCredentialName(), "WorkloadIdentityCredential");
+    WorkloadIdentityCredentialOptions options;
+    options.AuthorityHost = "bar";
+    EXPECT_EQ(options.AuthorityHost, "bar");
+  }
+
+  {
+    std::map<std::string, std::string> 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", ""}});
-
-  TokenRequestContext trc;
-  trc.Scopes.push_back("https://storage.azure.com/.default");
+  {
+    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");
+
+    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)