Allow x-vss-e2eid response header to be logged in AzurePipelinesCrede…

…ntial for diagnostics. (#6001) * Allow x-vss-e2eid response header to be logged in AzurePipelinesCredential for diagnostics. * Dont redact the x-msedge-ref header either. * Add the necessary response headers to the exception message. * Update cspell. * Update CL * Fix size_t comparison * Use std::array to get the size() method. * Add the <array> include directive to be explicit.
Azure · Sep 21, 2024 · 641dcc8 · 641dcc8
1 parent bc7feb0
commit 641dcc8
Show file tree

Hide file tree

Showing 5 changed files with 161 additions and 30 deletions.
diff --git 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
@@ -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
@@ -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<Azure::Core::Http::_internal::HttpPipeline> 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
@@ -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<HttpPipeline>(
+      optionsWithLoggableHeaders,
+      "identity",
+      PackageVersion::ToString(),
+      std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>>{},
+      std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>>{});
+
   m_oidcRequestUrl = _detail::DefaultOptionValues::GetOidcRequestUrl();
 
   if (serviceConnectionId.empty())
@@ -121,8 +132,21 @@ std::string AzurePipelinesCredential::GetOidcTokenResponse(
         + std::to_string(static_cast<std::underlying_type<decltype(statusCode)>::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<RawResponse> response = m_httpPipeline.Send(oidcRequest, context);
+  std::unique_ptr<RawResponse> 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
@@ -4,6 +4,9 @@
 #include "azure/identity/azure_pipelines_credential.hpp"
 #include "credential_test_helper.hpp"
 
+#include <azure/core/diagnostics/logger.hpp>
+
+#include <array>
 #include <cstdio>
 #include <fstream>
 
@@ -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<std::pair<Logger::Level, std::string>>;
+  LogMsgVec log;
+  Logger::SetLevel(Logger::Level::Verbose);
+  Logger::SetListener([&](auto lvl, auto msg) { log.push_back(std::make_pair(lvl, msg)); });
+
+  std::map<std::string, std::string> 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<CredentialTestHelper::TokenRequestSimulationServerResponse> 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<AzurePipelinesCredential>(
+            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<std::string, std::string> 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<std::string, 4> 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<std::string> const testScopes;
-    CredentialTestHelper::TokenRequestSimulationServerResponse testResponse;
-    testResponse.StatusCode = HttpStatusCode::BadRequest;
-    testResponse.Body = "Invalid response body";
-
-    static_cast<void>(CredentialTestHelper::SimulateTokenRequest(
-        [&](auto transport) {
-          AzurePipelinesCredentialOptions options;
-          options.Transport.Transport = transport;
+    try
+    {
 
-          return std::make_unique<AzurePipelinesCredential>(
-              tenantId, clientId, serviceConnectionId, systemAccessToken, options);
-        },
-        {testScopes},
-        {testResponse}));
+      std::vector<std::string> const testScopes;
+      CredentialTestHelper::TokenRequestSimulationServerResponse testResponse = testResponses[i];
 
-    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();
+      static_cast<void>(CredentialTestHelper::SimulateTokenRequest(
+          [&](auto transport) {
+            AzurePipelinesCredentialOptions options;
+            options.Transport.Transport = transport;
+
+            return std::make_unique<AzurePipelinesCredential>(
+                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