Improve AzurePipelinesCredential diagnosability (Azure#23485)

Author: chlowell

sdk/azidentity/CHANGELOG.md

@@ -9,6 +9,10 @@
 ### Bugs Fixed
 
 ### Other Changes
+* `AzurePipelinesCredential` sets an additional OIDC request header so that it
+  receives a 401 instead of a 302 after presenting an invalid system access token
+* Allow logging of debugging headers for `AzurePipelinesCredential` and include
+  them in error messages
 
 ## 1.8.0-beta.3 (2024-09-17)
 

sdk/azidentity/TROUBLESHOOTING.md

@@ -234,7 +234,7 @@ azd auth token --output json --scope https://management.core.windows.net/.defaul
 |---|---|---|
 | AADSTS900023: Specified tenant identifier 'some tenant ID' is neither a valid DNS name, nor a valid external domain.|The `tenantID` argument to `NewAzurePipelinesCredential` is incorrect| Verify the tenant ID. It must identify the tenant of the user-assigned managed identity or service principal configured for the service connection.|
 | No service connection found with identifier |The `serviceConnectionID` argument to `NewAzurePipelinesCredential` is incorrect| Verify the service connection ID. This parameter refers to the `resourceId` of the Azure Service Connection. It can also be found in the query string of the service connection's configuration in Azure DevOps. [Azure Pipelines documentation](https://learn.microsoft.com/azure/devops/pipelines/library/service-endpoints?view=azure-devops&tabs=yaml) has more information about service connections.|
-|302 (Found) response from OIDC endpoint|The `systemAccessToken` argument to `NewAzurePipelinesCredential` is incorrect|Check pipeline configuration. This value comes from the predefined variable `System.AccessToken` [as described in Azure Pipelines documentation](https://learn.microsoft.com/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml#systemaccesstoken).|
+|401 (Unauthorized) response from OIDC endpoint|The `systemAccessToken` argument to `NewAzurePipelinesCredential` is incorrect|Check pipeline configuration. This value comes from the predefined variable `System.AccessToken` [as described in Azure Pipelines documentation](https://learn.microsoft.com/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml#systemaccesstoken).|
 
 ## Get additional help
 

sdk/azidentity/azure_pipelines_credential.go

@@ -20,6 +20,8 @@ const (
 	credNameAzurePipelines = "AzurePipelinesCredential"
 	oidcAPIVersion         = "7.1"
 	systemOIDCRequestURI   = "SYSTEM_OIDCREQUESTURI"
+	xMsEdgeRef             = "x-msedge-ref"
+	xVssE2eId              = "x-vss-e2eid"
 )
 
 // AzurePipelinesCredential authenticates with workload identity federation in an Azure Pipeline. See
@@ -86,6 +88,8 @@ func NewAzurePipelinesCredential(tenantID, clientID, serviceConnectionID, system
 	if options == nil {
 		options = &AzurePipelinesCredentialOptions{}
 	}
+	// these headers are useful to the DevOps team when debugging OIDC error responses
+	options.ClientOptions.Logging.AllowedHeaders = append(options.ClientOptions.Logging.AllowedHeaders, xMsEdgeRef, xVssE2eId)
 	caco := ClientAssertionCredentialOptions{
 		AdditionallyAllowedTenants: options.AdditionallyAllowedTenants,
 		Cache:                      options.Cache,
@@ -121,12 +125,19 @@ func (a *AzurePipelinesCredential) getAssertion(ctx context.Context) (string, er
 		return "", newAuthenticationFailedError(credNameAzurePipelines, "couldn't create OIDC token request: "+err.Error(), nil)
 	}
 	req.Header.Set("Authorization", "Bearer "+a.systemAccessToken)
+	// instruct endpoint to return 401 instead of 302, if the system access token is invalid
+	req.Header.Set("X-TFS-FedAuthRedirect", "Suppress")
 	res, err := doForClient(a.cred.client.azClient, req)
 	if err != nil {
 		return "", newAuthenticationFailedError(credNameAzurePipelines, "couldn't send OIDC token request: "+err.Error(), nil)
 	}
 	if res.StatusCode != http.StatusOK {
-		msg := res.Status + " response from the OIDC endpoint. Check service connection ID and Pipeline configuration"
+		msg := res.Status + " response from the OIDC endpoint. Check service connection ID and Pipeline configuration."
+		for _, h := range []string{xMsEdgeRef, xVssE2eId} {
+			if v := res.Header.Get(h); v != "" {
+				msg += fmt.Sprintf("\n%s: %s", h, v)
+			}
+		}
 		// include the response because its body, if any, probably contains an error message.
 		// OK responses aren't included with errors because they probably contain secrets
 		return "", newAuthenticationFailedError(credNameAzurePipelines, msg, res)

sdk/azidentity/azure_pipelines_credential_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/log"
 	"github.com/Azure/azure-sdk-for-go/sdk/internal/mock"
 	"github.com/Azure/azure-sdk-for-go/sdk/internal/recording"
 	"github.com/stretchr/testify/require"
@@ -35,6 +36,7 @@ func TestAzurePipelinesCredential(t *testing.T) {
 				require.Equal(t, expected.Host, r.Host)
 				require.Equal(t, expected.Path, r.URL.Path)
 				require.Equal(t, expected.RawQuery, r.URL.RawQuery)
+				require.Equal(t, "Suppress", r.Header.Get("X-TFS-FedAuthRedirect"))
 				return true
 			}),
 		)
@@ -50,6 +52,53 @@ func TestAzurePipelinesCredential(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, tokenValue, actual)
 	})
+	t.Run("OIDC error headers", func(t *testing.T) {
+		expected := map[string]string{
+			xMsEdgeRef: "foo",
+			xVssE2eId:  "bar",
+		}
+		// for matching the expected headers in messages, canonicalized or not
+		regexFmt := `(?i)%s:\s+%s`
+
+		srv, close := mock.NewServer()
+		defer close()
+		t.Setenv(systemOIDCRequestURI, srv.URL())
+		ro := []mock.ResponseOption{mock.WithStatusCode(http.StatusUnauthorized)}
+		for k, v := range expected {
+			ro = append(ro, mock.WithHeader(k, v))
+		}
+		srv.AppendResponse(ro...)
+
+		logged := false
+		log.SetEvents(log.EventResponse)
+		log.SetListener(func(e log.Event, m string) {
+			if e == log.EventResponse {
+				logged = true
+				for k, v := range expected {
+					rx := fmt.Sprintf(regexFmt, k, v)
+					require.Regexp(t, rx, m, fmt.Sprintf(`expected header "%s: %s" in log message`, k, v))
+				}
+			}
+		})
+		defer func() {
+			log.SetEvents()
+			log.SetListener(nil)
+		}()
+
+		o := AzurePipelinesCredentialOptions{
+			ClientOptions: azcore.ClientOptions{
+				Transport: srv,
+			},
+		}
+		cred, err := NewAzurePipelinesCredential(fakeTenantID, fakeClientID, "connectionID", tokenValue, &o)
+		require.NoError(t, err)
+		_, err = cred.getAssertion(ctx)
+		for k, v := range expected {
+			rx := fmt.Sprintf(regexFmt, k, v)
+			require.Regexp(t, rx, err.Error(), fmt.Sprintf(`expected header "%s: %s" in error message`, k, v))
+		}
+		require.True(t, logged, "test bug: response should have been logged")
+	})
 	t.Run("Live", func(t *testing.T) {
 		if recording.GetRecordMode() != recording.LiveMode {
 			t.Skip("this test runs only live in an Azure Pipeline with a configured service connection")