diff --git a/.vscode/cspell.json b/.vscode/cspell.json index d92a1947a..0f3306f37 100644 --- a/.vscode/cspell.json +++ b/.vscode/cspell.json @@ -170,6 +170,7 @@ "morten", "moxygen", "MSAL", + "msedge", "msft", "msiexec", "MSRC", diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index 7fe92a6f7..cc398a1c5 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -10,6 +10,8 @@ ### Other Changes +- Allow certain response headers to be logged in `AzurePipelinesCredential` for diagnostics and include them in the exception message. + ## 1.10.0-beta.1 (2024-09-17) ### Features Added 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 fb50cffb2..656d566b0 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 @@ -58,7 +58,7 @@ namespace Azure { namespace Identity { private: std::string m_serviceConnectionId; std::string m_systemAccessToken; - Azure::Core::Http::_internal::HttpPipeline m_httpPipeline; + std::unique_ptr m_httpPipeline; std::string m_oidcRequestUrl; std::unique_ptr<_detail::ClientAssertionCredentialImpl> m_clientAssertionCredentialImpl; diff --git a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp index 3665fe6a5..0db0c29f8 100644 --- a/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp +++ b/sdk/identity/azure-identity/src/azure_pipelines_credential.cpp @@ -36,9 +36,20 @@ AzurePipelinesCredential::AzurePipelinesCredential( std::string systemAccessToken, AzurePipelinesCredentialOptions const& options) : TokenCredential("AzurePipelinesCredential"), m_serviceConnectionId(serviceConnectionId), - m_systemAccessToken(systemAccessToken), - m_httpPipeline(HttpPipeline(options, "identity", PackageVersion::ToString(), {}, {})) + m_systemAccessToken(systemAccessToken) { + // Allow these headers to be logged since they are used for troubleshooting. + AzurePipelinesCredentialOptions optionsWithLoggableHeaders = options; + optionsWithLoggableHeaders.Log.AllowedHttpHeaders.insert("x-vss-e2eid"); + optionsWithLoggableHeaders.Log.AllowedHttpHeaders.insert("x-msedge-ref"); + + m_httpPipeline = std::make_unique( + optionsWithLoggableHeaders, + "identity", + PackageVersion::ToString(), + std::vector>{}, + std::vector>{}); + m_oidcRequestUrl = _detail::DefaultOptionValues::GetOidcRequestUrl(); if (serviceConnectionId.empty()) @@ -121,8 +132,21 @@ std::string AzurePipelinesCredential::GetOidcTokenResponse( + std::to_string(static_cast::type>(statusCode)) + " (" + response->GetReasonPhrase() + ") response from the OIDC endpoint. Check service connection ID and Pipeline " - "configuration.\n\n" - + responseBody; + "configuration"; + + auto responseHeaders = response->GetHeaders(); + auto headerValue = responseHeaders.find("x-vss-e2eid"); + if (headerValue != responseHeaders.end()) + { + message += "\n" + headerValue->first + ":" + headerValue->second; + } + headerValue = responseHeaders.find("x-msedge-ref"); + if (headerValue != responseHeaders.end()) + { + message += "\n" + headerValue->first + ":" + headerValue->second; + } + message += "\n\n" + responseBody; + IdentityLog::Write(IdentityLog::Level::Verbose, message); throw AuthenticationException(message); @@ -158,7 +182,7 @@ AzurePipelinesCredential::~AzurePipelinesCredential() = default; std::string AzurePipelinesCredential::GetAssertion(Context const& context) const { Azure::Core::Http::Request oidcRequest = CreateOidcRequestMessage(); - std::unique_ptr response = m_httpPipeline.Send(oidcRequest, context); + std::unique_ptr response = m_httpPipeline->Send(oidcRequest, context); if (!response) { 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 07167abc7..df90ba3f4 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 @@ -4,6 +4,9 @@ #include "azure/identity/azure_pipelines_credential.hpp" #include "credential_test_helper.hpp" +#include + +#include #include #include @@ -13,6 +16,7 @@ using Azure::Core::_internal::Environment; using Azure::Core::Credentials::AccessToken; using Azure::Core::Credentials::AuthenticationException; using Azure::Core::Credentials::TokenRequestContext; +using Azure::Core::Diagnostics::Logger; using Azure::Core::Http::HttpMethod; using Azure::Identity::AzurePipelinesCredential; using Azure::Identity::AzurePipelinesCredentialOptions; @@ -151,6 +155,76 @@ TEST(AzurePipelinesCredential, InvalidArgs) } } +TEST(AzurePipelinesCredential, RegularExpectedHeadersLogged) +{ + using LogMsgVec = std::vector>; + LogMsgVec log; + Logger::SetLevel(Logger::Level::Verbose); + Logger::SetListener([&](auto lvl, auto msg) { log.push_back(std::make_pair(lvl, msg)); }); + + std::map validEnvVars + = {{"SYSTEM_OIDCREQUESTURI", "https://localhost/instance"}}; + CredentialTestHelper::EnvironmentOverride const env(validEnvVars); + + using namespace Azure::Identity::Test::_detail; + using Azure::Core::CaseInsensitiveMap; + using Azure::Core::Http::HttpStatusCode; + + // The first response is from the OIDC endpoint, the second is from the identity token endpoint. + // The x-vss-e2eid header should be logged in the first response, but not in the second. + CaseInsensitiveMap responseHeaders; + responseHeaders.emplace("x-vss-e2eid", "some id for debugging"); + responseHeaders.emplace("x-msedge-ref", "some AFD impression log reference"); + + CredentialTestHelper::TokenRequestSimulationServerResponse response1 + = {HttpStatusCode::Ok, "{\"oidcToken\":\"abc/d\"}", responseHeaders}; + + CredentialTestHelper::TokenRequestSimulationServerResponse response2 + = {HttpStatusCode::Ok, + "{\"expires_in\":3600, \"access_token\":\"ACCESSTOKEN1\"}", + responseHeaders}; + + std::vector responses + = {response1, response2}; + + auto const actual = CredentialTestHelper::SimulateTokenRequest( + [](auto transport) { + AzurePipelinesCredentialOptions options; + options.Transport.Transport = transport; + + std::string tenantId = "01234567-89ab-cdef-fedc-ba8976543210"; + std::string clientId = "fedcba98-7654-3210-0123-456789abcdef"; + std::string serviceConnectionId = "a/bc"; + std::string systemAccessToken = "123"; + + return std::make_unique( + tenantId, clientId, serviceConnectionId, systemAccessToken, options); + }, + {{{"https://azure.com/.default"}}}, + responses); + + auto const& response0 = actual.Responses.at(0); + + EXPECT_EQ(response0.AccessToken.Token, "ACCESSTOKEN1"); + + using namespace std::chrono_literals; + EXPECT_GE(response0.AccessToken.ExpiresOn, response0.EarliestExpiration + 3600s); + EXPECT_LE(response0.AccessToken.ExpiresOn, response0.LatestExpiration + 3600s); + + EXPECT_EQ(log.size(), LogMsgVec::size_type(7)); + // The first response, from the OIDC endpoint, should have the x-vss-e2eid header logged. + EXPECT_TRUE(log[2].second.find("some id for debugging") != std::string::npos); + EXPECT_TRUE(log[2].second.find("some AFD impression log reference") != std::string::npos); + + // The second response, from the identity token endpoint still has that header redacted, as + // expected. + EXPECT_TRUE(log[5].second.find("some id for debugging") == std::string::npos); + EXPECT_TRUE(log[5].second.find("some AFD impression log reference") == std::string::npos); + EXPECT_TRUE(log[5].second.find("REDACTED") != std::string::npos); + + Logger::SetListener(nullptr); +} + TEST(AzurePipelinesCredential, Regular) { std::map validEnvVars @@ -422,34 +496,64 @@ TEST(AzurePipelinesCredential, InvalidOidcResponse) std::string serviceConnectionId = "a/bc"; std::string systemAccessToken = "123"; + using Azure::Core::Http::HttpStatusCode; + // Non-OK response - try + CredentialTestHelper::TokenRequestSimulationServerResponse testResponse0; + testResponse0.StatusCode = HttpStatusCode::BadRequest; + testResponse0.Body = "Invalid response body"; + + CredentialTestHelper::TokenRequestSimulationServerResponse testResponse1 = testResponse0; + testResponse1.Headers.emplace("x-vss-e2eid", "some id for debugging"); + + CredentialTestHelper::TokenRequestSimulationServerResponse testResponse2 = testResponse0; + testResponse2.Headers.emplace("x-msedge-ref", "some AFD impression log reference"); + + CredentialTestHelper::TokenRequestSimulationServerResponse testResponse3 = testResponse0; + testResponse3.Headers.emplace("x-vss-e2eid", "some id for debugging"); + testResponse3.Headers.emplace("x-msedge-ref", "some AFD impression log reference"); + testResponse3.Headers.emplace("foo", "bar"); // won't show up in the exception message + + CredentialTestHelper::TokenRequestSimulationServerResponse testResponses[4] + = {testResponse0, testResponse1, testResponse2, testResponse3}; + + std::string baseExpectedMessage + = "AzurePipelinesCredential : 400 (Test) response from the OIDC endpoint. Check service " + "connection ID and Pipeline configuration"; + std::array expectedMessages + = {baseExpectedMessage + "\n\nInvalid response body", + baseExpectedMessage + "\nx-vss-e2eid:some id for debugging\n\nInvalid response body", + baseExpectedMessage + + "\nx-msedge-ref:some AFD impression log reference\n\nInvalid response body", + baseExpectedMessage + + "\nx-vss-e2eid:some id for debugging\nx-msedge-ref:some AFD impression log " + "reference\n\nInvalid response body"}; + + for (size_t i = 0; i < expectedMessages.size(); i++) { - using Azure::Core::Http::HttpStatusCode; - std::vector const testScopes; - CredentialTestHelper::TokenRequestSimulationServerResponse testResponse; - testResponse.StatusCode = HttpStatusCode::BadRequest; - testResponse.Body = "Invalid response body"; + try + { - static_cast(CredentialTestHelper::SimulateTokenRequest( - [&](auto transport) { - AzurePipelinesCredentialOptions options; - options.Transport.Transport = transport; + std::vector const testScopes; + CredentialTestHelper::TokenRequestSimulationServerResponse testResponse = testResponses[i]; - return std::make_unique( - tenantId, clientId, serviceConnectionId, systemAccessToken, options); - }, - {testScopes}, - {testResponse})); + static_cast(CredentialTestHelper::SimulateTokenRequest( + [&](auto transport) { + AzurePipelinesCredentialOptions options; + options.Transport.Transport = transport; - EXPECT_TRUE(!"AzurePipelinesCredential should throw given the response above."); - } - catch (AuthenticationException const& ex) - { - std::string expectedMessage - = "AzurePipelinesCredential : 400 (Test) response from the OIDC endpoint. Check service " - "connection ID and Pipeline configuration.\n\nInvalid response body"; - EXPECT_EQ(ex.what(), expectedMessage) << ex.what(); + return std::make_unique( + tenantId, clientId, serviceConnectionId, systemAccessToken, options); + }, + {testScopes}, + {testResponse})); + + EXPECT_TRUE(!"AzurePipelinesCredential should throw given the response above."); + } + catch (AuthenticationException const& ex) + { + EXPECT_EQ(ex.what(), expectedMessages[i]) << ex.what(); + } } // Invalid JSON