From b90a45ee721f4d4d91c33d36eb961f93f380b609 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 15 Jul 2024 21:28:04 -0700 Subject: [PATCH 1/8] Use ClientAssertionCredential within AzurePipelinesCredential. --- .../identity/azure_pipelines_credential.hpp | 9 +- .../src/azure_pipelines_credential.cpp | 106 +++--------------- .../src/client_assertion_credential.cpp | 25 +---- .../src/private/tenant_id_resolver.hpp | 2 + .../azure-identity/src/tenant_id_resolver.cpp | 21 ++++ 5 files changed, 45 insertions(+), 118 deletions(-) diff --git a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp index 0a96fc7b85..ab0f59358e 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp @@ -8,6 +8,7 @@ #pragma once +#include "azure/identity/client_assertion_credential.hpp" #include "azure/identity/detail/client_credential_core.hpp" #include "azure/identity/detail/token_cache.hpp" @@ -57,14 +58,12 @@ namespace Azure { namespace Identity { private: std::string m_serviceConnectionId; std::string m_systemAccessToken; - _detail::ClientCredentialCore m_clientCredentialCore; Azure::Core::Http::_internal::HttpPipeline m_httpPipeline; std::string m_oidcRequestUrl; - std::unique_ptr<_detail::TokenCredentialImpl> m_tokenCredentialImpl; - std::string m_requestBody; - _detail::TokenCache m_tokenCache; + std::unique_ptr m_clientAssertionCredential; + bool m_credentialCreatedSuccessfully; - std::string GetAssertion(Core::Context const& context) const; + std::string GetAssertion(Core::Context const& context); Azure::Core::Http::Request CreateOidcRequestMessage() const; std::string GetOidcTokenResponse( std::unique_ptr const& response, diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index 2fb8f0757a..735723fcba 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -30,29 +30,6 @@ using Azure::Identity::_detail::PackageVersion; using Azure::Identity::_detail::TenantIdResolver; using Azure::Identity::_detail::TokenCredentialImpl; -namespace { -bool IsValidTenantId(std::string const& tenantId) -{ - const std::string allowedChars = ".-"; - if (tenantId.empty()) - { - return false; - } - for (auto const c : tenantId) - { - if (allowedChars.find(c) != std::string::npos) - { - continue; - } - if (!StringExtensions::IsAlphaNumeric(c)) - { - return false; - } - } - return true; -} -} // namespace - AzurePipelinesCredential::AzurePipelinesCredential( std::string tenantId, std::string clientId, @@ -61,26 +38,21 @@ AzurePipelinesCredential::AzurePipelinesCredential( AzurePipelinesCredentialOptions const& options) : TokenCredential("AzurePipelinesCredential"), m_serviceConnectionId(serviceConnectionId), m_systemAccessToken(systemAccessToken), - m_clientCredentialCore(tenantId, options.AuthorityHost, options.AdditionallyAllowedTenants), m_httpPipeline(HttpPipeline(options, "identity", PackageVersion::ToString(), {}, {})) { m_oidcRequestUrl = _detail::DefaultOptionValues::GetOidcRequestUrl(); + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; + clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; + + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; + + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); - bool isTenantIdValid = IsValidTenantId(tenantId); - if (!isTenantIdValid) - { - IdentityLog::Write( - IdentityLog::Level::Warning, - "Invalid tenant ID provided for " + GetCredentialName() - + ". The tenant ID must be a non-empty string containing only alphanumeric characters, " - "periods, or hyphens. You can locate your tenant ID by following the instructions " - "listed here: https://learn.microsoft.com/partner-center/find-ids-and-domain-names"); - } - if (clientId.empty()) - { - IdentityLog::Write( - IdentityLog::Level::Warning, "No client ID specified for " + GetCredentialName() + "."); - } if (serviceConnectionId.empty()) { IdentityLog::Write( @@ -101,18 +73,10 @@ AzurePipelinesCredential::AzurePipelinesCredential( + "' needed by " + GetCredentialName() + ". This should be set by Azure Pipelines."); } - if (isTenantIdValid && !clientId.empty() && !serviceConnectionId.empty() - && !systemAccessToken.empty() && !m_oidcRequestUrl.empty()) + m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() + && !serviceConnectionId.empty() && !systemAccessToken.empty() && !m_oidcRequestUrl.empty(); + if (m_credentialCreatedSuccessfully) { - m_tokenCredentialImpl = std::make_unique(options); - m_requestBody - = std::string( - "grant_type=client_credentials" - "&client_assertion_type=" - "urn%3Aietf%3Aparams%3Aoauth%3Aclient-assertion-type%3Ajwt-bearer" // cspell:disable-line - "&client_id=") - + Url::Encode(clientId); - IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } @@ -191,7 +155,7 @@ std::string AzurePipelinesCredential::GetOidcTokenResponse( AzurePipelinesCredential::~AzurePipelinesCredential() = default; -std::string AzurePipelinesCredential::GetAssertion(Context const& context) const +std::string AzurePipelinesCredential::GetAssertion(Context const& context) { Azure::Core::Http::Request oidcRequest = CreateOidcRequestMessage(); std::unique_ptr response = m_httpPipeline.Send(oidcRequest, context); @@ -214,7 +178,7 @@ AccessToken AzurePipelinesCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_tokenCredentialImpl) + if (!m_credentialCreatedSuccessfully) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; @@ -226,41 +190,5 @@ AccessToken AzurePipelinesCredential::GetToken( AuthUnavailable + "Azure Pipelines environment is not set up correctly."); } - auto const tenantId = TenantIdResolver::Resolve( - m_clientCredentialCore.GetTenantId(), - tokenRequestContext, - m_clientCredentialCore.GetAdditionallyAllowedTenants()); - - auto const scopesStr - = m_clientCredentialCore.GetScopesString(tenantId, tokenRequestContext.Scopes); - - // TokenCache::GetToken() and m_tokenCredentialImpl->GetToken() can only use the lambda - // argument when they are being executed. They are not supposed to keep a reference to lambda - // argument to call it later. Therefore, any capture made here will outlive the possible time - // frame when the lambda might get called. - return m_tokenCache.GetToken(scopesStr, tenantId, tokenRequestContext.MinimumExpiration, [&]() { - return m_tokenCredentialImpl->GetToken(context, false, [&]() { - auto body = m_requestBody; - if (!scopesStr.empty()) - { - body += "&scope=" + scopesStr; - } - - // Get the request url before calling GetAssertion to validate the authority host scheme. - // This is to avoid making a request to the OIDC endpoint if the authority host scheme is - // invalid. - auto const requestUrl = m_clientCredentialCore.GetRequestUrl(tenantId); - - const std::string assertion = GetAssertion(context); - - body += "&client_assertion=" + Azure::Core::Url::Encode(assertion); - - auto request - = std::make_unique(HttpMethod::Post, requestUrl, body); - - request->HttpRequest.SetHeader("Host", requestUrl.GetHost()); - - return request; - }); - }); + return m_clientAssertionCredential->GetToken(tokenRequestContext, context); } diff --git a/sdk/identity/azure-identity/src/client_assertion_credential.cpp b/sdk/identity/azure-identity/src/client_assertion_credential.cpp index 596a06576b..4fc1d29b72 100644 --- a/sdk/identity/azure-identity/src/client_assertion_credential.cpp +++ b/sdk/identity/azure-identity/src/client_assertion_credential.cpp @@ -24,29 +24,6 @@ using Azure::Identity::_detail::IdentityLog; using Azure::Identity::_detail::TenantIdResolver; using Azure::Identity::_detail::TokenCredentialImpl; -namespace { -bool IsValidTenantId(std::string const& tenantId) -{ - const std::string allowedChars = ".-"; - if (tenantId.empty()) - { - return false; - } - for (auto const c : tenantId) - { - if (allowedChars.find(c) != std::string::npos) - { - continue; - } - if (!StringExtensions::IsAlphaNumeric(c)) - { - return false; - } - } - return true; -} -} // namespace - ClientAssertionCredential::ClientAssertionCredential( std::string tenantId, std::string clientId, @@ -56,7 +33,7 @@ ClientAssertionCredential::ClientAssertionCredential( m_assertionCallback(std::move(assertionCallback)), m_clientCredentialCore(tenantId, options.AuthorityHost, options.AdditionallyAllowedTenants) { - bool isTenantIdValid = IsValidTenantId(tenantId); + bool isTenantIdValid = TenantIdResolver::IsValidTenantId(tenantId); if (!isTenantIdValid) { IdentityLog::Write( diff --git a/sdk/identity/azure-identity/src/private/tenant_id_resolver.hpp b/sdk/identity/azure-identity/src/private/tenant_id_resolver.hpp index 4e6d7d4616..8a17b211c2 100644 --- a/sdk/identity/azure-identity/src/private/tenant_id_resolver.hpp +++ b/sdk/identity/azure-identity/src/private/tenant_id_resolver.hpp @@ -25,5 +25,7 @@ namespace Azure { namespace Identity { namespace _detail { // ADFS is the Active Directory Federation Service, a tenant ID that is used in Azure Stack. static bool IsAdfs(std::string const& tenantId); + + static bool IsValidTenantId(std::string const& tenantId); }; }}} // namespace Azure::Identity::_detail diff --git a/sdk/identity/azure-identity/src/tenant_id_resolver.cpp b/sdk/identity/azure-identity/src/tenant_id_resolver.cpp index ff88f56b03..fd89c78bb7 100644 --- a/sdk/identity/azure-identity/src/tenant_id_resolver.cpp +++ b/sdk/identity/azure-identity/src/tenant_id_resolver.cpp @@ -56,3 +56,24 @@ bool TenantIdResolver::IsAdfs(std::string const& tenantId) { return StringExtensions::LocaleInvariantCaseInsensitiveEqual(tenantId, "adfs"); } + +bool TenantIdResolver::IsValidTenantId(std::string const& tenantId) +{ + const std::string allowedChars = ".-"; + if (tenantId.empty()) + { + return false; + } + for (auto const c : tenantId) + { + if (allowedChars.find(c) != std::string::npos) + { + continue; + } + if (!StringExtensions::IsAlphaNumeric(c)) + { + return false; + } + } + return true; +} From 75a52f6a25748144d85a1044a4bf4d1d82af9b43 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 15 Jul 2024 21:57:18 -0700 Subject: [PATCH 2/8] Use ClientAssertionCredential in WorkloadIdentityCredential. --- .../identity/workload_identity_credential.hpp | 9 +- .../src/workload_identity_credential.cpp | 115 +++++++----------- 2 files changed, 46 insertions(+), 78 deletions(-) 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 b1e9798e3f..a5c16e93f5 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 @@ -8,6 +8,7 @@ #pragma once +#include "azure/identity/client_assertion_credential.hpp" #include "azure/identity/detail/client_credential_core.hpp" #include "azure/identity/detail/token_cache.hpp" @@ -74,11 +75,11 @@ namespace Azure { namespace Identity { */ class WorkloadIdentityCredential final : public Core::Credentials::TokenCredential { private: - _detail::TokenCache m_tokenCache; - _detail::ClientCredentialCore m_clientCredentialCore; - std::unique_ptr<_detail::TokenCredentialImpl> m_tokenCredentialImpl; - std::string m_requestBody; + std::unique_ptr m_clientAssertionCredential; std::string m_tokenFilePath; + bool m_credentialCreatedSuccessfully; + + std::string GetAssertion(Core::Context const& context); public: /** diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index 2fe0cd2132..fdf5a542f0 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -27,29 +27,28 @@ using Azure::Identity::_detail::TokenCredentialImpl; WorkloadIdentityCredential::WorkloadIdentityCredential( WorkloadIdentityCredentialOptions const& options) - : TokenCredential("WorkloadIdentityCredential"), m_clientCredentialCore( - options.TenantId, - options.AuthorityHost, - options.AdditionallyAllowedTenants) + : TokenCredential("WorkloadIdentityCredential") { std::string tenantId = options.TenantId; std::string clientId = options.ClientId; - std::string authorityHost = options.AuthorityHost; m_tokenFilePath = options.TokenFilePath; - if (!tenantId.empty() && !clientId.empty() && !m_tokenFilePath.empty()) - { - m_clientCredentialCore = Azure::Identity::_detail::ClientCredentialCore( - tenantId, authorityHost, options.AdditionallyAllowedTenants); - m_tokenCredentialImpl = std::make_unique(options); - m_requestBody - = std::string( - "grant_type=client_credentials" - "&client_assertion_type=" - "urn%3Aietf%3Aparams%3Aoauth%3Aclient-assertion-type%3Ajwt-bearer" // cspell:disable-line - "&client_id=") - + Url::Encode(clientId); + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; + clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; + + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; + + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); + m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() + && !m_tokenFilePath.empty(); + if (m_credentialCreatedSuccessfully) + { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } @@ -63,29 +62,25 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( } WorkloadIdentityCredential::WorkloadIdentityCredential( - Core::Credentials::TokenCredentialOptions const& options) - : TokenCredential("WorkloadIdentityCredential"), - m_clientCredentialCore("", "", std::vector()) + Core::Credentials::TokenCredentialOptions const&) + : TokenCredential("WorkloadIdentityCredential") { 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 const authorityHost = _detail::DefaultOptionValues::GetAuthorityHost(); - - m_clientCredentialCore = Azure::Identity::_detail::ClientCredentialCore( - tenantId, authorityHost, std::vector()); - m_tokenCredentialImpl = std::make_unique(options); - m_requestBody - = std::string( - "grant_type=client_credentials" - "&client_assertion_type=" - "urn%3Aietf%3Aparams%3Aoauth%3Aclient-assertion-type%3Ajwt-bearer" // cspell:disable-line - "&client_id=") - + Url::Encode(clientId); + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential + = std::make_unique(tenantId, clientId, callback); + + m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() + && !m_tokenFilePath.empty(); + if (m_credentialCreatedSuccessfully) + { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } @@ -100,11 +95,21 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( WorkloadIdentityCredential::~WorkloadIdentityCredential() = default; +std::string WorkloadIdentityCredential::GetAssertion(Context const&) +{ + // Read the specified file's content, which is expected to be a Kubernetes service account + // token. Kubernetes is responsible for updating the file as service account tokens expire. + std::ifstream azureFederatedTokenFile(m_tokenFilePath); + std::string assertion( + (std::istreambuf_iterator(azureFederatedTokenFile)), std::istreambuf_iterator()); + return assertion; +} + AccessToken WorkloadIdentityCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_tokenCredentialImpl) + if (!m_credentialCreatedSuccessfully) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; @@ -116,43 +121,5 @@ AccessToken WorkloadIdentityCredential::GetToken( AuthUnavailable + "Azure Kubernetes environment is not set up correctly."); } - auto const tenantId = TenantIdResolver::Resolve( - m_clientCredentialCore.GetTenantId(), - tokenRequestContext, - m_clientCredentialCore.GetAdditionallyAllowedTenants()); - - auto const scopesStr - = m_clientCredentialCore.GetScopesString(tenantId, tokenRequestContext.Scopes); - - // TokenCache::GetToken() and m_tokenCredentialImpl->GetToken() can only use the lambda argument - // when they are being executed. They are not supposed to keep a reference to lambda argument to - // call it later. Therefore, any capture made here will outlive the possible time frame when the - // lambda might get called. - return m_tokenCache.GetToken(scopesStr, tenantId, tokenRequestContext.MinimumExpiration, [&]() { - return m_tokenCredentialImpl->GetToken(context, false, [&]() { - auto body = m_requestBody; - if (!scopesStr.empty()) - { - body += "&scope=" + scopesStr; - } - - auto const requestUrl = m_clientCredentialCore.GetRequestUrl(tenantId); - - // Read the specified file's content, which is expected to be a Kubernetes service account - // token. Kubernetes is responsible for updating the file as service account tokens expire. - std::ifstream azureFederatedTokenFile(m_tokenFilePath); - std::string assertion( - (std::istreambuf_iterator(azureFederatedTokenFile)), - std::istreambuf_iterator()); - - body += "&client_assertion=" + Azure::Core::Url::Encode(assertion); - - auto request - = std::make_unique(HttpMethod::Post, requestUrl, body); - - request->HttpRequest.SetHeader("Host", requestUrl.GetHost()); - - return request; - }); - }); + return m_clientAssertionCredential->GetToken(tokenRequestContext, context); } From d02e9cf3df2b0c165c4179be3bae7cca882570a0 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 15 Jul 2024 22:17:46 -0700 Subject: [PATCH 3/8] Fix DefaultAzureCredentia.LogMessages test since an extra log got added. --- .../test/ut/default_azure_credential_test.cpp | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp index d571952384..14aa970284 100644 --- a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp @@ -169,7 +169,7 @@ TEST(DefaultAzureCredential, LogMessages) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(11)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(12)); EXPECT_EQ(log[0].first, Logger::Level::Verbose); EXPECT_EQ( @@ -196,48 +196,51 @@ TEST(DefaultAzureCredential, LogMessages) "authorityHost gets created."); EXPECT_EQ(log[3].first, Logger::Level::Informational); - EXPECT_EQ(log[3].second, "Identity: WorkloadIdentityCredential was created successfully."); + EXPECT_EQ(log[3].second, "Identity: ClientAssertionCredential was created successfully."); - EXPECT_EQ(log[5].first, Logger::Level::Verbose); - EXPECT_EQ( - log[5].second, - "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2019 source."); + EXPECT_EQ(log[4].first, Logger::Level::Informational); + EXPECT_EQ(log[4].second, "Identity: WorkloadIdentityCredential was created successfully."); EXPECT_EQ(log[6].first, Logger::Level::Verbose); EXPECT_EQ( log[6].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2017 source."); + "to be created with App Service 2019 source."); EXPECT_EQ(log[7].first, Logger::Level::Verbose); EXPECT_EQ( log[7].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with Cloud Shell source."); + "to be created with App Service 2017 source."); EXPECT_EQ(log[8].first, Logger::Level::Verbose); EXPECT_EQ( log[8].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with Azure Arc source."); + "to be created with Cloud Shell source."); - EXPECT_EQ(log[9].first, Logger::Level::Informational); + EXPECT_EQ(log[9].first, Logger::Level::Verbose); EXPECT_EQ( log[9].second, + "Identity: ManagedIdentityCredential: Environment is not set up for the credential " + "to be created with Azure Arc source."); + + EXPECT_EQ(log[10].first, Logger::Level::Informational); + EXPECT_EQ( + log[10].second, "Identity: ManagedIdentityCredential will be created " "with Azure Instance Metadata Service source." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[4].first, Logger::Level::Informational); + EXPECT_EQ(log[5].first, Logger::Level::Informational); EXPECT_EQ( - log[4].second, + log[5].second, "Identity: AzureCliCredential created." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[10].first, Logger::Level::Informational); + EXPECT_EQ(log[11].first, Logger::Level::Informational); EXPECT_EQ( - log[10].second, + log[11].second, "Identity: DefaultAzureCredential: Created with the following credentials: " "EnvironmentCredential, WorkloadIdentityCredential, AzureCliCredential, " "ManagedIdentityCredential."); From 80149f55ac9e48d7f3e0e146434c6a5c1cb94604 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Mon, 15 Jul 2024 22:52:44 -0700 Subject: [PATCH 4/8] Disable tests that dont correctly simulate the token request and return the test response. --- .../test/ut/azure_pipelines_credential_test.cpp | 6 +++--- .../test/ut/workload_identity_credential_test.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp index 8d5280be6a..335bd2ddb4 100644 --- a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp @@ -151,7 +151,7 @@ TEST(AzurePipelinesCredential, InvalidArgs) } } -TEST(AzurePipelinesCredential, Regular) +TEST(AzurePipelinesCredential, DISABLED_Regular) { std::map validEnvVars = {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}}; @@ -228,7 +228,7 @@ TEST(AzurePipelinesCredential, Regular) EXPECT_LE(response0.AccessToken.ExpiresOn, response0.LatestExpiration + 3600s); } -TEST(AzurePipelinesCredential, AzureStack) +TEST(AzurePipelinesCredential, DISABLED_AzureStack) { std::map validEnvVars = {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}}; @@ -303,7 +303,7 @@ TEST(AzurePipelinesCredential, AzureStack) EXPECT_LE(response0.AccessToken.ExpiresOn, response0.LatestExpiration + 3600s); } -TEST(AzurePipelinesCredential, Authority) +TEST(AzurePipelinesCredential, DISABLED_Authority) { CredentialTestHelper::EnvironmentOverride const env( {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}, 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 64106f59fd..62d2c5d3cd 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 @@ -126,7 +126,7 @@ TEST(WorkloadIdentityCredential, GetOptionsFromEnvironmentInvalid) } } -TEST(WorkloadIdentityCredential, Regular) +TEST(WorkloadIdentityCredential, DISABLED_Regular) { TempCertFile const tempCertFile; @@ -213,7 +213,7 @@ TEST(WorkloadIdentityCredential, Regular) EXPECT_LE(response1.AccessToken.ExpiresOn, response1.LatestExpiration + 7200s); } -TEST(WorkloadIdentityCredential, AzureStack) +TEST(WorkloadIdentityCredential, DISABLED_AzureStack) { TempCertFile const tempCertFile; @@ -296,7 +296,7 @@ TEST(WorkloadIdentityCredential, AzureStack) EXPECT_LE(response1.AccessToken.ExpiresOn, response1.LatestExpiration + 7200s); } -TEST(WorkloadIdentityCredential, Authority) +TEST(WorkloadIdentityCredential, DISABLED_Authority) { TempCertFile const tempCertFile; From 7e8813420b8f5ef2a6bd5eff2325779f2355bbfe Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 16 Jul 2024 22:54:37 -0700 Subject: [PATCH 5/8] Address PR feedback and make sure base options are passed in to underlying client assertion credential. --- .../identity/azure_pipelines_credential.hpp | 1 - .../identity/workload_identity_credential.hpp | 1 - .../src/azure_pipelines_credential.cpp | 14 +++++--- .../src/workload_identity_credential.cpp | 32 +++++++++++++------ .../ut/azure_pipelines_credential_test.cpp | 6 ++-- .../ut/workload_identity_credential_test.cpp | 6 ++-- 6 files changed, 38 insertions(+), 22 deletions(-) diff --git a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp index ab0f59358e..132991b8e2 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp @@ -61,7 +61,6 @@ namespace Azure { namespace Identity { Azure::Core::Http::_internal::HttpPipeline m_httpPipeline; std::string m_oidcRequestUrl; std::unique_ptr m_clientAssertionCredential; - bool m_credentialCreatedSuccessfully; std::string GetAssertion(Core::Context const& context); Azure::Core::Http::Request CreateOidcRequestMessage() const; 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 a5c16e93f5..b1d96eaf08 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 @@ -77,7 +77,6 @@ namespace Azure { namespace Identity { private: std::unique_ptr m_clientAssertionCredential; std::string m_tokenFilePath; - bool m_credentialCreatedSuccessfully; std::string GetAssertion(Core::Context const& context); diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index 735723fcba..99bbe59347 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -42,6 +42,9 @@ AzurePipelinesCredential::AzurePipelinesCredential( { m_oidcRequestUrl = _detail::DefaultOptionValues::GetOidcRequestUrl(); ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; @@ -73,15 +76,18 @@ AzurePipelinesCredential::AzurePipelinesCredential( + "' needed by " + GetCredentialName() + ". This should be set by Azure Pipelines."); } - m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() - && !serviceConnectionId.empty() && !systemAccessToken.empty() && !m_oidcRequestUrl.empty(); - if (m_credentialCreatedSuccessfully) + if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() + && !serviceConnectionId.empty() && !systemAccessToken.empty() && !m_oidcRequestUrl.empty()) { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { + // We use this to indicate that the credential is not usable, so that we can throw when used in + // GetToken(). + m_clientAssertionCredential.reset(); + // Rather than throwing an exception in the ctor, following the pattern in existing credentials // to log the errors, and defer throwing an exception to the first call of GetToken(). This is // primarily needed for credentials that are part of the DefaultAzureCredential, which this @@ -178,7 +184,7 @@ AccessToken AzurePipelinesCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_credentialCreatedSuccessfully) + if (!m_clientAssertionCredential) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index fdf5a542f0..b9cb720443 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -34,6 +34,9 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( m_tokenFilePath = options.TokenFilePath; ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; @@ -45,15 +48,17 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( m_clientAssertionCredential = std::make_unique( tenantId, clientId, callback, clientAssertionCredentialOptions); - m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() - && !m_tokenFilePath.empty(); - if (m_credentialCreatedSuccessfully) + if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !m_tokenFilePath.empty()) { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { + // We use this to indicate that the credential is not usable, so that we can throw when used in + // GetToken(). + m_clientAssertionCredential.reset(); + IdentityLog::Write( IdentityLog::Level::Warning, "Azure Kubernetes environment is not set up for the " + GetCredentialName() @@ -62,30 +67,37 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( } WorkloadIdentityCredential::WorkloadIdentityCredential( - Core::Credentials::TokenCredentialOptions const&) + Core::Credentials::TokenCredentialOptions const& options) : TokenCredential("WorkloadIdentityCredential") { std::string const tenantId = _detail::DefaultOptionValues::GetTenantId(); std::string const clientId = _detail::DefaultOptionValues::GetClientId(); m_tokenFilePath = _detail::DefaultOptionValues::GetFederatedTokenFile(); + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; + std::function callback = [this](Context const& context) { return GetAssertion(context); }; // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs // warning messages otherwise. - m_clientAssertionCredential - = std::make_unique(tenantId, clientId, callback); + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); - m_credentialCreatedSuccessfully = TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() - && !m_tokenFilePath.empty(); - if (m_credentialCreatedSuccessfully) + if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !m_tokenFilePath.empty()) { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { + // We use this to indicate that the credential is not usable, so that we can throw when used in + // GetToken(). + m_clientAssertionCredential.reset(); + IdentityLog::Write( IdentityLog::Level::Warning, "Azure Kubernetes environment is not set up for the " + GetCredentialName() @@ -109,7 +121,7 @@ AccessToken WorkloadIdentityCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_credentialCreatedSuccessfully) + if (!m_clientAssertionCredential) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; diff --git a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp index 335bd2ddb4..8d5280be6a 100644 --- a/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/azure_pipelines_credential_test.cpp @@ -151,7 +151,7 @@ TEST(AzurePipelinesCredential, InvalidArgs) } } -TEST(AzurePipelinesCredential, DISABLED_Regular) +TEST(AzurePipelinesCredential, Regular) { std::map validEnvVars = {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}}; @@ -228,7 +228,7 @@ TEST(AzurePipelinesCredential, DISABLED_Regular) EXPECT_LE(response0.AccessToken.ExpiresOn, response0.LatestExpiration + 3600s); } -TEST(AzurePipelinesCredential, DISABLED_AzureStack) +TEST(AzurePipelinesCredential, AzureStack) { std::map validEnvVars = {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}}; @@ -303,7 +303,7 @@ TEST(AzurePipelinesCredential, DISABLED_AzureStack) EXPECT_LE(response0.AccessToken.ExpiresOn, response0.LatestExpiration + 3600s); } -TEST(AzurePipelinesCredential, DISABLED_Authority) +TEST(AzurePipelinesCredential, Authority) { CredentialTestHelper::EnvironmentOverride const env( {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}, 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 62d2c5d3cd..64106f59fd 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 @@ -126,7 +126,7 @@ TEST(WorkloadIdentityCredential, GetOptionsFromEnvironmentInvalid) } } -TEST(WorkloadIdentityCredential, DISABLED_Regular) +TEST(WorkloadIdentityCredential, Regular) { TempCertFile const tempCertFile; @@ -213,7 +213,7 @@ TEST(WorkloadIdentityCredential, DISABLED_Regular) EXPECT_LE(response1.AccessToken.ExpiresOn, response1.LatestExpiration + 7200s); } -TEST(WorkloadIdentityCredential, DISABLED_AzureStack) +TEST(WorkloadIdentityCredential, AzureStack) { TempCertFile const tempCertFile; @@ -296,7 +296,7 @@ TEST(WorkloadIdentityCredential, DISABLED_AzureStack) EXPECT_LE(response1.AccessToken.ExpiresOn, response1.LatestExpiration + 7200s); } -TEST(WorkloadIdentityCredential, DISABLED_Authority) +TEST(WorkloadIdentityCredential, Authority) { TempCertFile const tempCertFile; From 525b9b24a1e5eb50a7aab11b2b7d5a8d238f4722 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 16 Jul 2024 23:05:27 -0700 Subject: [PATCH 6/8] Address PR feedback - move credential ctor into validation checks. --- .../src/azure_pipelines_credential.cpp | 34 +++++----- .../src/workload_identity_credential.cpp | 63 +++++++++---------- 2 files changed, 44 insertions(+), 53 deletions(-) diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index 99bbe59347..e5f3fc9586 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -41,20 +41,6 @@ AzurePipelinesCredential::AzurePipelinesCredential( m_httpPipeline(HttpPipeline(options, "identity", PackageVersion::ToString(), {}, {})) { m_oidcRequestUrl = _detail::DefaultOptionValues::GetOidcRequestUrl(); - ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; - // Get the options from the base class (including ClientOptions). - static_cast(clientAssertionCredentialOptions) - = options; - clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; - clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; - - std::function callback - = [this](Context const& context) { return GetAssertion(context); }; - - // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs - // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); if (serviceConnectionId.empty()) { @@ -79,15 +65,27 @@ AzurePipelinesCredential::AzurePipelinesCredential( if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !serviceConnectionId.empty() && !systemAccessToken.empty() && !m_oidcRequestUrl.empty()) { + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; + clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; + clientAssertionCredentialOptions.AdditionallyAllowedTenants + = options.AdditionallyAllowedTenants; + + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; + + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); + IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { - // We use this to indicate that the credential is not usable, so that we can throw when used in - // GetToken(). - m_clientAssertionCredential.reset(); - // Rather than throwing an exception in the ctor, following the pattern in existing credentials // to log the errors, and defer throwing an exception to the first call of GetToken(). This is // primarily needed for credentials that are part of the DefaultAzureCredential, which this diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index b9cb720443..590c8ab774 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -33,32 +33,29 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( std::string clientId = options.ClientId; m_tokenFilePath = options.TokenFilePath; - ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; - // Get the options from the base class (including ClientOptions). - static_cast(clientAssertionCredentialOptions) - = options; - clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; - clientAssertionCredentialOptions.AdditionallyAllowedTenants = options.AdditionallyAllowedTenants; - - std::function callback - = [this](Context const& context) { return GetAssertion(context); }; - - // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs - // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); - if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !m_tokenFilePath.empty()) { + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; + clientAssertionCredentialOptions.AuthorityHost = options.AuthorityHost; + clientAssertionCredentialOptions.AdditionallyAllowedTenants + = options.AdditionallyAllowedTenants; + + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; + + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); + IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { - // We use this to indicate that the credential is not usable, so that we can throw when used in - // GetToken(). - m_clientAssertionCredential.reset(); - IdentityLog::Write( IdentityLog::Level::Warning, "Azure Kubernetes environment is not set up for the " + GetCredentialName() @@ -74,30 +71,26 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( std::string const clientId = _detail::DefaultOptionValues::GetClientId(); m_tokenFilePath = _detail::DefaultOptionValues::GetFederatedTokenFile(); - ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; - // Get the options from the base class (including ClientOptions). - static_cast(clientAssertionCredentialOptions) - = options; + if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !m_tokenFilePath.empty()) + { + ClientAssertionCredentialOptions clientAssertionCredentialOptions{}; + // Get the options from the base class (including ClientOptions). + static_cast(clientAssertionCredentialOptions) + = options; - std::function callback - = [this](Context const& context) { return GetAssertion(context); }; + std::function callback + = [this](Context const& context) { return GetAssertion(context); }; - // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs - // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); + // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs + // warning messages otherwise. + m_clientAssertionCredential = std::make_unique( + tenantId, clientId, callback, clientAssertionCredentialOptions); - if (TenantIdResolver::IsValidTenantId(tenantId) && !clientId.empty() && !m_tokenFilePath.empty()) - { IdentityLog::Write( IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); } else { - // We use this to indicate that the credential is not usable, so that we can throw when used in - // GetToken(). - m_clientAssertionCredential.reset(); - IdentityLog::Write( IdentityLog::Level::Warning, "Azure Kubernetes environment is not set up for the " + GetCredentialName() From 4f3f6a4a2d88f32a32776899befbe213c412f7cb Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 2 Aug 2024 13:35:25 -0700 Subject: [PATCH 7/8] Address PR feedback, add const. --- .../inc/azure/identity/azure_pipelines_credential.hpp | 2 +- .../inc/azure/identity/workload_identity_credential.hpp | 2 +- sdk/identity/azure-identity/src/azure_pipelines_credential.cpp | 2 +- .../azure-identity/src/workload_identity_credential.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp index 132991b8e2..c0e7adb7fb 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp @@ -62,7 +62,7 @@ namespace Azure { namespace Identity { std::string m_oidcRequestUrl; std::unique_ptr m_clientAssertionCredential; - std::string GetAssertion(Core::Context const& context); + std::string GetAssertion(Core::Context const& context) const; Azure::Core::Http::Request CreateOidcRequestMessage() const; std::string GetOidcTokenResponse( std::unique_ptr const& response, 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 b1d96eaf08..4bd0d05c01 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 @@ -78,7 +78,7 @@ namespace Azure { namespace Identity { std::unique_ptr m_clientAssertionCredential; std::string m_tokenFilePath; - std::string GetAssertion(Core::Context const& context); + std::string GetAssertion(Core::Context const& context) const; public: /** diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index e5f3fc9586..89143496df 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -159,7 +159,7 @@ std::string AzurePipelinesCredential::GetOidcTokenResponse( AzurePipelinesCredential::~AzurePipelinesCredential() = default; -std::string AzurePipelinesCredential::GetAssertion(Context const& context) +std::string AzurePipelinesCredential::GetAssertion(Context const& context) const { Azure::Core::Http::Request oidcRequest = CreateOidcRequestMessage(); std::unique_ptr response = m_httpPipeline.Send(oidcRequest, context); diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index 590c8ab774..924ed72601 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -100,7 +100,7 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( WorkloadIdentityCredential::~WorkloadIdentityCredential() = default; -std::string WorkloadIdentityCredential::GetAssertion(Context const&) +std::string WorkloadIdentityCredential::GetAssertion(Context const&) const { // Read the specified file's content, which is expected to be a Kubernetes service account // token. Kubernetes is responsible for updating the file as service account tokens expire. From c8ac51a784abca8655576e330a4dbb77e0ad36ea Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Fri, 2 Aug 2024 14:11:54 -0700 Subject: [PATCH 8/8] Add a ClientAssertionCredentialImpl to make sure logs use the calling credential name. --- sdk/identity/azure-identity/CMakeLists.txt | 1 + .../identity/azure_pipelines_credential.hpp | 4 +- .../identity/client_assertion_credential.hpp | 10 +--- .../identity/workload_identity_credential.hpp | 4 +- .../src/azure_pipelines_credential.cpp | 15 +++--- .../src/client_assertion_credential.cpp | 53 +++++++++++++------ .../client_assertion_credential_impl.hpp | 42 +++++++++++++++ .../src/workload_identity_credential.cpp | 22 +++----- .../test/ut/default_azure_credential_test.cpp | 33 ++++++------ 9 files changed, 116 insertions(+), 68 deletions(-) create mode 100644 sdk/identity/azure-identity/src/private/client_assertion_credential_impl.hpp diff --git a/sdk/identity/azure-identity/CMakeLists.txt b/sdk/identity/azure-identity/CMakeLists.txt index 2a3ffd8c49..34d3832c6f 100644 --- a/sdk/identity/azure-identity/CMakeLists.txt +++ b/sdk/identity/azure-identity/CMakeLists.txt @@ -77,6 +77,7 @@ set( src/managed_identity_credential.cpp src/managed_identity_source.cpp src/private/chained_token_credential_impl.hpp + src/private/client_assertion_credential_impl.hpp src/private/identity_log.hpp src/private/managed_identity_source.hpp src/private/package_version.hpp diff --git a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp index c0e7adb7fb..fb50cffb2e 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/azure_pipelines_credential.hpp @@ -21,7 +21,7 @@ namespace Azure { namespace Identity { namespace _detail { - class TokenCredentialImpl; + class ClientAssertionCredentialImpl; } // namespace _detail /** @@ -60,7 +60,7 @@ namespace Azure { namespace Identity { std::string m_systemAccessToken; Azure::Core::Http::_internal::HttpPipeline m_httpPipeline; std::string m_oidcRequestUrl; - std::unique_ptr m_clientAssertionCredential; + std::unique_ptr<_detail::ClientAssertionCredentialImpl> m_clientAssertionCredentialImpl; std::string GetAssertion(Core::Context const& context) const; Azure::Core::Http::Request CreateOidcRequestMessage() const; diff --git a/sdk/identity/azure-identity/inc/azure/identity/client_assertion_credential.hpp b/sdk/identity/azure-identity/inc/azure/identity/client_assertion_credential.hpp index e1fd875528..3a9af365aa 100644 --- a/sdk/identity/azure-identity/inc/azure/identity/client_assertion_credential.hpp +++ b/sdk/identity/azure-identity/inc/azure/identity/client_assertion_credential.hpp @@ -9,17 +9,15 @@ #pragma once #include "azure/identity/detail/client_credential_core.hpp" -#include "azure/identity/detail/token_cache.hpp" #include -#include #include #include namespace Azure { namespace Identity { namespace _detail { - class TokenCredentialImpl; + class ClientAssertionCredentialImpl; } // namespace _detail /** @@ -55,11 +53,7 @@ namespace Azure { namespace Identity { */ class ClientAssertionCredential final : public Core::Credentials::TokenCredential { private: - std::function m_assertionCallback; - _detail::ClientCredentialCore m_clientCredentialCore; - std::unique_ptr<_detail::TokenCredentialImpl> m_tokenCredentialImpl; - std::string m_requestBody; - _detail::TokenCache m_tokenCache; + std::unique_ptr<_detail::ClientAssertionCredentialImpl> m_impl; public: /** 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 4bd0d05c01..0e481e99c5 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 @@ -19,7 +19,7 @@ namespace Azure { namespace Identity { namespace _detail { - class TokenCredentialImpl; + class ClientAssertionCredentialImpl; } // namespace _detail /** @@ -75,7 +75,7 @@ namespace Azure { namespace Identity { */ class WorkloadIdentityCredential final : public Core::Credentials::TokenCredential { private: - std::unique_ptr m_clientAssertionCredential; + std::unique_ptr<_detail::ClientAssertionCredentialImpl> m_clientAssertionCredentialImpl; std::string m_tokenFilePath; std::string GetAssertion(Core::Context const& context) const; diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index 89143496df..3665fe6a5b 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -3,10 +3,10 @@ #include "azure/identity/azure_pipelines_credential.hpp" +#include "private/client_assertion_credential_impl.hpp" #include "private/identity_log.hpp" #include "private/package_version.hpp" #include "private/tenant_id_resolver.hpp" -#include "private/token_credential_impl.hpp" #include @@ -28,7 +28,6 @@ using Azure::Core::Json::_internal::json; using Azure::Identity::_detail::IdentityLog; using Azure::Identity::_detail::PackageVersion; using Azure::Identity::_detail::TenantIdResolver; -using Azure::Identity::_detail::TokenCredentialImpl; AzurePipelinesCredential::AzurePipelinesCredential( std::string tenantId, @@ -78,11 +77,8 @@ AzurePipelinesCredential::AzurePipelinesCredential( // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); - - IdentityLog::Write( - IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); + m_clientAssertionCredentialImpl = std::make_unique<_detail::ClientAssertionCredentialImpl>( + GetCredentialName(), tenantId, clientId, callback, clientAssertionCredentialOptions); } else { @@ -182,7 +178,7 @@ AccessToken AzurePipelinesCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_clientAssertionCredential) + if (!m_clientAssertionCredentialImpl) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; @@ -194,5 +190,6 @@ AccessToken AzurePipelinesCredential::GetToken( AuthUnavailable + "Azure Pipelines environment is not set up correctly."); } - return m_clientAssertionCredential->GetToken(tokenRequestContext, context); + return m_clientAssertionCredentialImpl->GetToken( + GetCredentialName(), tokenRequestContext, context); } diff --git a/sdk/identity/azure-identity/src/client_assertion_credential.cpp b/sdk/identity/azure-identity/src/client_assertion_credential.cpp index 4fc1d29b72..8224516847 100644 --- a/sdk/identity/azure-identity/src/client_assertion_credential.cpp +++ b/sdk/identity/azure-identity/src/client_assertion_credential.cpp @@ -3,15 +3,16 @@ #include "azure/identity/client_assertion_credential.hpp" +#include "private/client_assertion_credential_impl.hpp" #include "private/identity_log.hpp" #include "private/package_version.hpp" #include "private/tenant_id_resolver.hpp" -#include "private/token_credential_impl.hpp" #include using Azure::Identity::ClientAssertionCredential; using Azure::Identity::ClientAssertionCredentialOptions; +using Azure::Identity::_detail::ClientAssertionCredentialImpl; using Azure::Core::Context; using Azure::Core::Url; @@ -24,13 +25,13 @@ using Azure::Identity::_detail::IdentityLog; using Azure::Identity::_detail::TenantIdResolver; using Azure::Identity::_detail::TokenCredentialImpl; -ClientAssertionCredential::ClientAssertionCredential( +ClientAssertionCredentialImpl::ClientAssertionCredentialImpl( + std::string const& credentialName, std::string tenantId, std::string clientId, std::function assertionCallback, ClientAssertionCredentialOptions const& options) - : TokenCredential("ClientAssertionCredential"), - m_assertionCallback(std::move(assertionCallback)), + : m_assertionCallback(std::move(assertionCallback)), m_clientCredentialCore(tenantId, options.AuthorityHost, options.AdditionallyAllowedTenants) { bool isTenantIdValid = TenantIdResolver::IsValidTenantId(tenantId); @@ -38,7 +39,7 @@ ClientAssertionCredential::ClientAssertionCredential( { IdentityLog::Write( IdentityLog::Level::Warning, - GetCredentialName() + credentialName + ": Invalid tenant ID provided. The tenant ID must be a non-empty string containing " "only alphanumeric characters, periods, or hyphens. You can locate your tenant ID by " "following the instructions listed here: " @@ -46,14 +47,13 @@ ClientAssertionCredential::ClientAssertionCredential( } if (clientId.empty()) { - IdentityLog::Write( - IdentityLog::Level::Warning, GetCredentialName() + ": No client ID specified."); + IdentityLog::Write(IdentityLog::Level::Warning, credentialName + ": No client ID specified."); } if (!m_assertionCallback) { IdentityLog::Write( IdentityLog::Level::Warning, - GetCredentialName() + credentialName + ": The assertionCallback must be a valid function that returns assertions."); } @@ -69,7 +69,7 @@ ClientAssertionCredential::ClientAssertionCredential( + Url::Encode(clientId); IdentityLog::Write( - IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); + IdentityLog::Level::Informational, credentialName + " was created successfully."); } else { @@ -78,23 +78,22 @@ ClientAssertionCredential::ClientAssertionCredential( // primarily needed for credentials that are part of the DefaultAzureCredential, which this // credential is not intended for. IdentityLog::Write( - IdentityLog::Level::Warning, GetCredentialName() + " was not initialized correctly."); + IdentityLog::Level::Warning, credentialName + " was not initialized correctly."); } } -ClientAssertionCredential::~ClientAssertionCredential() = default; - -AccessToken ClientAssertionCredential::GetToken( +AccessToken ClientAssertionCredentialImpl::GetToken( + std::string const& credentialName, TokenRequestContext const& tokenRequestContext, Context const& context) const { if (!m_tokenCredentialImpl) { - auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; + auto const AuthUnavailable = credentialName + " authentication unavailable. "; IdentityLog::Write( IdentityLog::Level::Warning, - AuthUnavailable + "See earlier " + GetCredentialName() + " log messages for details."); + AuthUnavailable + "See earlier " + credentialName + " log messages for details."); throw AuthenticationException(AuthUnavailable); } @@ -137,3 +136,27 @@ AccessToken ClientAssertionCredential::GetToken( }); }); } + +ClientAssertionCredential::ClientAssertionCredential( + std::string tenantId, + std::string clientId, + std::function assertionCallback, + ClientAssertionCredentialOptions const& options) + : TokenCredential("ClientAssertionCredential"), + m_impl(std::make_unique( + GetCredentialName(), + tenantId, + clientId, + assertionCallback, + options)) +{ +} + +ClientAssertionCredential::~ClientAssertionCredential() = default; + +AccessToken ClientAssertionCredential::GetToken( + TokenRequestContext const& tokenRequestContext, + Context const& context) const +{ + return m_impl->GetToken(GetCredentialName(), tokenRequestContext, context); +} diff --git a/sdk/identity/azure-identity/src/private/client_assertion_credential_impl.hpp b/sdk/identity/azure-identity/src/private/client_assertion_credential_impl.hpp new file mode 100644 index 0000000000..58144d753c --- /dev/null +++ b/sdk/identity/azure-identity/src/private/client_assertion_credential_impl.hpp @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * @file + * @brief Client Assertion Credential and options. + */ + +#pragma once + +#include "azure/identity/client_assertion_credential.hpp" +#include "azure/identity/detail/client_credential_core.hpp" +#include "azure/identity/detail/token_cache.hpp" +#include "token_credential_impl.hpp" + +#include + +namespace Azure { namespace Identity { namespace _detail { + class TokenCredentialImpl; + + class ClientAssertionCredentialImpl final { + private: + std::function m_assertionCallback; + _detail::ClientCredentialCore m_clientCredentialCore; + std::unique_ptr m_tokenCredentialImpl; + std::string m_requestBody; + _detail::TokenCache m_tokenCache; + + public: + ClientAssertionCredentialImpl( + std::string const& credentialName, + std::string tenantId, + std::string clientId, + std::function assertionCallback, + ClientAssertionCredentialOptions const& options = {}); + + Core::Credentials::AccessToken GetToken( + std::string const& credentialName, + Core::Credentials::TokenRequestContext const& tokenRequestContext, + Core::Context const& context) const; + }; +}}} // namespace Azure::Identity::_detail diff --git a/sdk/identity/azure-identity/src/workload_identity_credential.cpp b/sdk/identity/azure-identity/src/workload_identity_credential.cpp index 924ed72601..59711966b4 100644 --- a/sdk/identity/azure-identity/src/workload_identity_credential.cpp +++ b/sdk/identity/azure-identity/src/workload_identity_credential.cpp @@ -3,9 +3,9 @@ #include "azure/identity/workload_identity_credential.hpp" +#include "private/client_assertion_credential_impl.hpp" #include "private/identity_log.hpp" #include "private/tenant_id_resolver.hpp" -#include "private/token_credential_impl.hpp" #include @@ -23,7 +23,6 @@ using Azure::Core::Credentials::TokenRequestContext; using Azure::Core::Http::HttpMethod; using Azure::Identity::_detail::IdentityLog; using Azure::Identity::_detail::TenantIdResolver; -using Azure::Identity::_detail::TokenCredentialImpl; WorkloadIdentityCredential::WorkloadIdentityCredential( WorkloadIdentityCredentialOptions const& options) @@ -48,11 +47,8 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); - - IdentityLog::Write( - IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); + m_clientAssertionCredentialImpl = std::make_unique<_detail::ClientAssertionCredentialImpl>( + GetCredentialName(), tenantId, clientId, callback, clientAssertionCredentialOptions); } else { @@ -83,11 +79,8 @@ WorkloadIdentityCredential::WorkloadIdentityCredential( // ClientAssertionCredential validates the tenant ID, client ID, and assertion callback and logs // warning messages otherwise. - m_clientAssertionCredential = std::make_unique( - tenantId, clientId, callback, clientAssertionCredentialOptions); - - IdentityLog::Write( - IdentityLog::Level::Informational, GetCredentialName() + " was created successfully."); + m_clientAssertionCredentialImpl = std::make_unique<_detail::ClientAssertionCredentialImpl>( + GetCredentialName(), tenantId, clientId, callback, clientAssertionCredentialOptions); } else { @@ -114,7 +107,7 @@ AccessToken WorkloadIdentityCredential::GetToken( TokenRequestContext const& tokenRequestContext, Context const& context) const { - if (!m_clientAssertionCredential) + if (!m_clientAssertionCredentialImpl) { auto const AuthUnavailable = GetCredentialName() + " authentication unavailable. "; @@ -126,5 +119,6 @@ AccessToken WorkloadIdentityCredential::GetToken( AuthUnavailable + "Azure Kubernetes environment is not set up correctly."); } - return m_clientAssertionCredential->GetToken(tokenRequestContext, context); + return m_clientAssertionCredentialImpl->GetToken( + GetCredentialName(), tokenRequestContext, context); } diff --git a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp index 14aa970284..d571952384 100644 --- a/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp +++ b/sdk/identity/azure-identity/test/ut/default_azure_credential_test.cpp @@ -169,7 +169,7 @@ TEST(DefaultAzureCredential, LogMessages) auto credential = std::make_unique(options); - EXPECT_EQ(log.size(), LogMsgVec::size_type(12)); + EXPECT_EQ(log.size(), LogMsgVec::size_type(11)); EXPECT_EQ(log[0].first, Logger::Level::Verbose); EXPECT_EQ( @@ -196,51 +196,48 @@ TEST(DefaultAzureCredential, LogMessages) "authorityHost gets created."); EXPECT_EQ(log[3].first, Logger::Level::Informational); - EXPECT_EQ(log[3].second, "Identity: ClientAssertionCredential was created successfully."); + EXPECT_EQ(log[3].second, "Identity: WorkloadIdentityCredential was created successfully."); - EXPECT_EQ(log[4].first, Logger::Level::Informational); - EXPECT_EQ(log[4].second, "Identity: WorkloadIdentityCredential was created successfully."); + EXPECT_EQ(log[5].first, Logger::Level::Verbose); + EXPECT_EQ( + log[5].second, + "Identity: ManagedIdentityCredential: Environment is not set up for the credential " + "to be created with App Service 2019 source."); EXPECT_EQ(log[6].first, Logger::Level::Verbose); EXPECT_EQ( log[6].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2019 source."); + "to be created with App Service 2017 source."); EXPECT_EQ(log[7].first, Logger::Level::Verbose); EXPECT_EQ( log[7].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with App Service 2017 source."); + "to be created with Cloud Shell source."); EXPECT_EQ(log[8].first, Logger::Level::Verbose); EXPECT_EQ( log[8].second, "Identity: ManagedIdentityCredential: Environment is not set up for the credential " - "to be created with Cloud Shell source."); - - EXPECT_EQ(log[9].first, Logger::Level::Verbose); - EXPECT_EQ( - log[9].second, - "Identity: ManagedIdentityCredential: Environment is not set up for the credential " "to be created with Azure Arc source."); - EXPECT_EQ(log[10].first, Logger::Level::Informational); + EXPECT_EQ(log[9].first, Logger::Level::Informational); EXPECT_EQ( - log[10].second, + log[9].second, "Identity: ManagedIdentityCredential will be created " "with Azure Instance Metadata Service source." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[5].first, Logger::Level::Informational); + EXPECT_EQ(log[4].first, Logger::Level::Informational); EXPECT_EQ( - log[5].second, + log[4].second, "Identity: AzureCliCredential created." "\nSuccessful creation does not guarantee further successful token retrieval."); - EXPECT_EQ(log[11].first, Logger::Level::Informational); + EXPECT_EQ(log[10].first, Logger::Level::Informational); EXPECT_EQ( - log[11].second, + log[10].second, "Identity: DefaultAzureCredential: Created with the following credentials: " "EnvironmentCredential, WorkloadIdentityCredential, AzureCliCredential, " "ManagedIdentityCredential.");