fix: honor retry.max_delay as outbound request timeout

GitOrigin-RevId: 9f3419b5d328caf2385bc4d71ac33f3b0094cd1c

Author: aeneasr

pipeline/authn/authenticator_oauth2_client_credentials.go

@@ -93,7 +93,7 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (*
 	if err != nil {
 		return nil, err
 	}
-	timeout := time.Millisecond * duration
+	timeout := duration
 	a.client = httpx.NewResilientClient(
 		httpx.ResilientClientWithMaxRetryWait(maxWait),
 		httpx.ResilientClientWithConnectionTimeout(timeout),
@@ -220,7 +220,7 @@ func (a *AuthenticatorOAuth2ClientCredentials) Authenticate(r *http.Request, ses
 		t, err := c.Token(context.WithValue(
 			r.Context(),
 			oauth2.HTTPClient,
-			c.Client,
+			a.client,
 		))
 		if err != nil {
 			if rErr, ok := err.(*oauth2.RetrieveError); ok {

pipeline/authn/authenticator_oauth2_client_credentials_test.go

@@ -269,10 +269,10 @@ func TestAuthenticatorOAuth2ClientCredentials(t *testing.T) {
 			},
 		},
 		{
-			d:         "fails and returns 503 Service Unavailable error due to the unavailability of the upstream service",
+			d:         "fails and returns not available after the resilient client retries a persistent 503",
 			r:         upstreamFailure,
 			expectErr: helper.ErrUpstreamServiceNotAvailable(),
-			config:    json.RawMessage(`{}`),
+			config:    json.RawMessage(`{"retry":{"give_up_after":"10ms"}}`),
 			token_url: "",
 			setup: func(t *testing.T, h *http.ServeMux, _ json.RawMessage) {
 				h.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, r *http.Request) {
@@ -282,10 +282,13 @@ func TestAuthenticatorOAuth2ClientCredentials(t *testing.T) {
 			},
 		},
 		{
-			d:         "fails and returns 504 Gateway Timeout error due to upstream service timeout",
+			// The resilient client retries the 504 and, once retries are
+			// exhausted, surfaces the upstream as not available (the specific
+			// status is no longer available after give-up).
+			d:         "fails and returns not available after the resilient client retries a persistent 504",
 			r:         upstreamFailure,
-			expectErr: helper.ErrUpstreamServiceTimeout(),
-			config:    json.RawMessage(`{}`),
+			expectErr: helper.ErrUpstreamServiceNotAvailable(),
+			config:    json.RawMessage(`{"retry":{"give_up_after":"10ms"}}`),
 			token_url: "",
 			setup: func(t *testing.T, h *http.ServeMux, _ json.RawMessage) {
 				h.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, r *http.Request) {
@@ -295,10 +298,12 @@ func TestAuthenticatorOAuth2ClientCredentials(t *testing.T) {
 			},
 		},
 		{
-			d:         "fails and returns 500 Internal Server Error error due to an unexpected error in the upstream service",
+			// The resilient client retries the 500 and, once retries are
+			// exhausted, surfaces the upstream as not available.
+			d:         "fails and returns not available after the resilient client retries a persistent 500",
 			r:         upstreamFailure,
-			expectErr: helper.ErrUpstreamServiceInternalServerError(),
-			config:    json.RawMessage(`{}`),
+			expectErr: helper.ErrUpstreamServiceNotAvailable(),
+			config:    json.RawMessage(`{"retry":{"give_up_after":"10ms"}}`),
 			token_url: "",
 			setup: func(t *testing.T, h *http.ServeMux, _ json.RawMessage) {
 				h.HandleFunc("POST /oauth2/token", func(w http.ResponseWriter, r *http.Request) {
@@ -398,3 +403,45 @@ func TestAuthenticatorOAuth2ClientCredentials(t *testing.T) {
 		require.NoError(t, a.Validate(json.RawMessage(`{"token_url":"`+ts.URL+"/oauth2/token"+`","retry":{"give_up_after":"3s", "max_delay":"100ms"}}`)))
 	})
 }
+
+// TestAuthenticatorOAuth2ClientCredentialsHonorsMaxDelayTimeout is a regression
+// test for two coupled bugs that stopped retry.max_delay from being enforced as
+// the outbound HTTP timeout for the token request:
+//
+//   - The parsed max_delay duration was multiplied by an extra factor of
+//     time.Millisecond, inflating the timeout by 1e6 (a 50ms setting became
+//     ~13.9 hours).
+//   - The resilient client that carries the timeout (a.client) was never passed
+//     to the OAuth2 token exchange; a method value (c.Client) was passed
+//     instead, which the OAuth2 library ignores, so the default client with no
+//     timeout was used.
+//
+// With either bug present, a call to a slow token endpoint blocks until the
+// server responds; with both fixed, it times out promptly.
+func TestAuthenticatorOAuth2ClientCredentialsHonorsMaxDelayTimeout(t *testing.T) {
+	t.Parallel()
+	reg := internal.NewRegistry(t, configx.SkipValidation())
+	a, err := reg.PipelineAuthenticator("oauth2_client_credentials")
+	require.NoError(t, err)
+
+	slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		time.Sleep(2 * time.Second)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"access_token":"foo","token_type":"bearer"}`))
+	}))
+	t.Cleanup(slow.Close)
+
+	r := &http.Request{Header: http.Header{}}
+	r.SetBasicAuth("client", "secret")
+	config, err := sjson.SetBytes(
+		[]byte(`{"retry":{"max_delay":"50ms","give_up_after":"10ms"}}`), "token_url", slow.URL+"/oauth2/token")
+	require.NoError(t, err)
+
+	start := time.Now()
+	err = a.Authenticate(r, new(authn.AuthenticationSession), config, nil)
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.Less(t, elapsed, time.Second,
+		"the configured 50ms max_delay must time the token request out; the inflated value would block ~2s on the slow server")
+}

pipeline/authn/authenticator_oauth2_introspection.go

@@ -19,6 +19,7 @@ import (
 	"github.com/pkg/errors"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.opentelemetry.io/otel/trace"
+	"golang.org/x/oauth2"
 	"golang.org/x/oauth2/clientcredentials"
 
 	"github.com/ory/fosite"
@@ -321,22 +322,6 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth
 
 	if !ok || client == nil {
 		a.d.Logger().Debug("Initializing http client")
-		var rt http.RoundTripper
-		if c.PreAuth != nil && c.PreAuth.Enabled {
-			var ep url.Values
-
-			if c.PreAuth.Audience != "" {
-				ep = url.Values{"audience": {c.PreAuth.Audience}}
-			}
-
-			rt = (&clientcredentials.Config{
-				ClientID:       c.PreAuth.ClientID,
-				ClientSecret:   c.PreAuth.ClientSecret,
-				Scopes:         c.PreAuth.Scope,
-				EndpointParams: ep,
-				TokenURL:       c.PreAuth.TokenURL,
-			}).Client(context.Background()).Transport
-		}
 
 		if c.Retry == nil {
 			c.Retry = &AuthenticatorOAuth2IntrospectionRetryConfiguration{Timeout: "500ms", MaxWait: "1s"}
@@ -348,22 +333,49 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth
 				c.Retry.MaxWait = "1s"
 			}
 		}
-		duration, err := time.ParseDuration(c.Retry.Timeout)
+		timeout, err := time.ParseDuration(c.Retry.Timeout)
 		if err != nil {
 			return nil, nil, errors.WithStack(err)
 		}
-		timeout := time.Millisecond * duration
-
 		maxWait, err := time.ParseDuration(c.Retry.MaxWait)
 		if err != nil {
 			return nil, nil, errors.WithStack(err)
 		}
 
-		client = httpx.NewResilientClient(
+		// The resilient client enforces the configured retry.max_delay as the
+		// per-request connection timeout and retries transient failures. Requests
+		// must flow through it so those settings are actually honored.
+		resilient := httpx.NewResilientClient(
 			httpx.ResilientClientWithMaxRetryWait(maxWait),
 			httpx.ResilientClientWithConnectionTimeout(timeout),
 		).StandardClient()
-		client.Transport = otelhttp.NewTransport(rt, otelhttp.WithTracerProvider(a.d.Tracer(context.Background()).Provider()))
+
+		rt := resilient.Transport
+		if c.PreAuth != nil && c.PreAuth.Enabled {
+			var ep url.Values
+
+			if c.PreAuth.Audience != "" {
+				ep = url.Values{"audience": {c.PreAuth.Audience}}
+			}
+
+			// Perform the pre-authorization token exchange through the resilient
+			// client (via the oauth2.HTTPClient context value), and route the
+			// pre-authorized introspection requests through it as well by making
+			// the OAuth2 transport wrap the resilient transport.
+			preAuthCtx := context.WithValue(context.Background(), oauth2.HTTPClient, resilient)
+			rt = (&clientcredentials.Config{
+				ClientID:       c.PreAuth.ClientID,
+				ClientSecret:   c.PreAuth.ClientSecret,
+				Scopes:         c.PreAuth.Scope,
+				EndpointParams: ep,
+				TokenURL:       c.PreAuth.TokenURL,
+			}).Client(preAuthCtx).Transport
+		}
+
+		// Use a fresh outer client for tracing so the OAuth2 token source keeps
+		// using the resilient client above without recursing through the tracing
+		// transport.
+		client = &http.Client{Transport: otelhttp.NewTransport(rt, otelhttp.WithTracerProvider(a.d.Tracer(context.Background()).Provider()))}
 		a.mu.Lock()
 		a.clientMap[clientKey] = client
 		a.mu.Unlock()

pipeline/authn/authenticator_oauth2_introspection_test.go

@@ -898,3 +898,43 @@ func TestAudienceUnmarshal(t *testing.T) {
 		})
 	}
 }
+
+// TestAuthenticatorOAuth2IntrospectionRetryTimeout is a regression test for two
+// coupled bugs that stopped retry.max_delay from being enforced as the outbound
+// HTTP timeout for the introspection request:
+//
+//   - The parsed max_delay duration was multiplied by an extra factor of
+//     time.Millisecond, inflating the timeout by 1e6 (a 50ms setting became
+//     ~13.9 hours).
+//   - The resilient client that carries the timeout was discarded: its transport
+//     was overwritten with a tracing transport that wrapped a nil round tripper,
+//     so requests bypassed the timeout and retries entirely.
+//
+// With either bug present, a call to a slow introspection endpoint blocks until
+// the server responds; with both fixed, it times out promptly.
+func TestAuthenticatorOAuth2IntrospectionRetryTimeout(t *testing.T) {
+	t.Parallel()
+	reg := internal.NewRegistry(t, configx.SkipValidation())
+	a := NewAuthenticatorOAuth2Introspection(reg)
+
+	slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		time.Sleep(2 * time.Second)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"active":true}`))
+	}))
+	t.Cleanup(slow.Close)
+
+	config, err := sjson.SetBytes(
+		[]byte(`{"scope_strategy":"none","retry":{"max_delay":"50ms","give_up_after":"10ms"}}`),
+		"introspection_url", slow.URL+"/oauth2/introspect")
+	require.NoError(t, err)
+
+	r := &http.Request{Header: http.Header{"Authorization": {"bearer token"}}}
+	start := time.Now()
+	err = a.Authenticate(r, new(AuthenticationSession), config, nil)
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.Less(t, elapsed, time.Second,
+		"the configured 50ms max_delay must time the request out; the inflated value would block ~2s on the slow server")
+}

pipeline/authz/remote.go

@@ -175,7 +175,7 @@ func (a *AuthorizerRemote) Config(config json.RawMessage) (*AuthorizerRemoteConf
 	if err != nil {
 		return nil, err
 	}
-	timeout := time.Millisecond * duration
+	timeout := duration
 	client := httpx.NewResilientClient(
 		httpx.ResilientClientWithMaxRetryWait(maxWait),
 		httpx.ResilientClientWithConnectionTimeout(timeout),

pipeline/authz/remote_json.go

@@ -186,7 +186,7 @@ func (a *AuthorizerRemoteJSON) Config(config json.RawMessage) (*AuthorizerRemote
 	if err != nil {
 		return nil, err
 	}
-	timeout := time.Millisecond * duration
+	timeout := duration
 	client := httpx.NewResilientClient(
 		httpx.ResilientClientWithMaxRetryWait(maxWait),
 		httpx.ResilientClientWithConnectionTimeout(timeout),

pipeline/authz/remote_json_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -408,3 +409,36 @@ func TestAuthorizerRemoteJSONTracePropagation(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotEmpty(t, gotTraceparent, "expected traceparent header to be propagated to remote_json authorizer endpoint")
 }
+
+// TestAuthorizerRemoteJSONHonorsMaxDelayTimeout is a regression test for a bug
+// where the parsed retry.max_delay duration was multiplied by an extra factor
+// of time.Millisecond, inflating the outbound HTTP timeout by 1e6 (a 50ms
+// setting became ~13.9 hours, effectively disabling the timeout). With the bug
+// the call blocks until the slow server responds; with the fix it times out
+// promptly.
+func TestAuthorizerRemoteJSONHonorsMaxDelayTimeout(t *testing.T) {
+	t.Parallel()
+
+	slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		time.Sleep(2 * time.Second)
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(slow.Close)
+
+	config, err := sjson.SetBytes(
+		[]byte(`{"payload":"{}","retry":{"max_delay":"50ms","give_up_after":"10ms"}}`), "remote", slow.URL)
+	require.NoError(t, err)
+
+	reg := internal.NewRegistry(t)
+	a := NewAuthorizerRemoteJSON(reg)
+	r, err := http.NewRequestWithContext(t.Context(), "POST", "", nil)
+	require.NoError(t, err)
+
+	start := time.Now()
+	err = a.Authorize(r, &authn.AuthenticationSession{}, config, &rule.Rule{})
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.Less(t, elapsed, time.Second,
+		"the configured 50ms max_delay must time the request out; the inflated value would block ~2s on the slow server")
+}

pipeline/authz/remote_test.go

@@ -13,6 +13,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -337,3 +338,36 @@ func TestAuthorizerRemoteTracePropagation(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotEmpty(t, gotTraceparent, "expected traceparent header to be propagated to remote authorizer endpoint")
 }
+
+// TestAuthorizerRemoteHonorsMaxDelayTimeout is a regression test for a bug where
+// the parsed retry.max_delay duration was multiplied by an extra factor of
+// time.Millisecond, inflating the outbound HTTP timeout by 1e6 (a 50ms setting
+// became ~13.9 hours, effectively disabling the timeout). With the bug the call
+// blocks until the slow server responds; with the fix it times out promptly.
+func TestAuthorizerRemoteHonorsMaxDelayTimeout(t *testing.T) {
+	t.Parallel()
+
+	slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		time.Sleep(2 * time.Second)
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(slow.Close)
+
+	config, err := sjson.SetBytes(
+		[]byte(`{"retry":{"max_delay":"50ms","give_up_after":"10ms"}}`), "remote", slow.URL)
+	require.NoError(t, err)
+
+	reg := internal.NewRegistry(t)
+	a := NewAuthorizerRemote(reg)
+	r, err := http.NewRequestWithContext(t.Context(), "POST", "", nil)
+	require.NoError(t, err)
+	r.Header.Set("Content-Type", "text/plain")
+
+	start := time.Now()
+	err = a.Authorize(r, &authn.AuthenticationSession{}, config, &rule.Rule{})
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.Less(t, elapsed, time.Second,
+		"the configured 50ms max_delay must time the request out; the inflated value would block ~2s on the slow server")
+}