From fb493f5c86b3fb3c12ee288950da11d8534ff36c Mon Sep 17 00:00:00 2001 From: Brandon Domingue Date: Mon, 12 Oct 2026 17:14:34 -0400 Subject: [PATCH] feat(http): support OAuth2 token request timeouts --- pkg/http/client/BUILD.bazel | 17 +- pkg/http/client/oauth2_token_source.go | 12 ++ pkg/http/client/oauth2_token_source_test.go | 196 ++++++++++++++++++ .../configuration/http/client/BUILD.bazel | 1 + .../configuration/http/client/client.pb.go | 42 ++-- .../configuration/http/client/client.proto | 11 + 6 files changed, 263 insertions(+), 16 deletions(-) create mode 100644 pkg/http/client/oauth2_token_source_test.go diff --git a/pkg/http/client/BUILD.bazel b/pkg/http/client/BUILD.bazel index ec8f40b36..65eb36e8b 100644 --- a/pkg/http/client/BUILD.bazel +++ b/pkg/http/client/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "client", @@ -22,3 +22,18 @@ go_library( "@org_golang_x_oauth2//clientcredentials", ], ) + +go_test( + name = "client_test", + size = "small", + srcs = ["oauth2_token_source_test.go"], + deps = [ + ":client", + "//pkg/proto/configuration/http/client", + "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//status", + "@org_golang_google_protobuf//encoding/protojson", + "@org_golang_google_protobuf//types/known/durationpb", + "@org_golang_x_oauth2//:oauth2", + ], +) diff --git a/pkg/http/client/oauth2_token_source.go b/pkg/http/client/oauth2_token_source.go index d170ef923..07f546f62 100644 --- a/pkg/http/client/oauth2_token_source.go +++ b/pkg/http/client/oauth2_token_source.go @@ -3,6 +3,7 @@ package client import ( "context" "net/http" + "time" configuration "github.com/buildbarn/bb-storage/pkg/proto/configuration/http/client" "github.com/buildbarn/bb-storage/pkg/util" @@ -32,12 +33,23 @@ func NewOAuth2TokenSourceFromConfiguration(oauthConfig *configuration.OAuth2Conf } func clientCredentialsTokenSource(scopes []string, config *configuration.OAuth2Configuration_ClientCredentials) (oauth2.TokenSource, error) { + var timeout time.Duration + if tokenRequestTimeout := config.ClientCredentials.TokenRequestTimeout; tokenRequestTimeout != nil { + if err := tokenRequestTimeout.CheckValid(); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid token request timeout: %s", err) + } + timeout = tokenRequestTimeout.AsDuration() + if timeout <= 0 { + return nil, status.Error(codes.InvalidArgument, "Token request timeout must be positive") + } + } roundTripper, err := NewRoundTripperFromConfiguration(config.ClientCredentials.HttpClient) if err != nil { return nil, util.StatusWrap(err, "Failed to create HTTP client") } httpClient := &http.Client{ Transport: NewMetricsRoundTripper(roundTripper, "ClientCredentials"), + Timeout: timeout, } oidcConfig := &clientcredentials.Config{ ClientID: config.ClientCredentials.ClientId, diff --git a/pkg/http/client/oauth2_token_source_test.go b/pkg/http/client/oauth2_token_source_test.go new file mode 100644 index 000000000..23fffeb1f --- /dev/null +++ b/pkg/http/client/oauth2_token_source_test.go @@ -0,0 +1,196 @@ +package client_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + httpclient "github.com/buildbarn/bb-storage/pkg/http/client" + configuration "github.com/buildbarn/bb-storage/pkg/proto/configuration/http/client" + + "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/types/known/durationpb" +) + +func tokenSource(timeout *durationpb.Duration, endpoint string) (oauth2.TokenSource, error) { + return httpclient.NewOAuth2TokenSourceFromConfiguration(&configuration.OAuth2Configuration{ + Credentials: &configuration.OAuth2Configuration_ClientCredentials{ + ClientCredentials: &configuration.OAuth2ClientCredentials{ + ClientId: "synthetic-client", + ClientSecret: "synthetic-secret", + TokenEndpointUrl: endpoint, + TokenRequestTimeout: timeout, + }, + }, + }) +} + +func TestTokenRequestTimeoutValidation(t *testing.T) { + for _, tc := range []struct { + name string + timeout *durationpb.Duration + code codes.Code + }{ + {name: "unset", code: codes.OK}, + {name: "positive", timeout: durationpb.New(15 * time.Second), code: codes.OK}, + {name: "zero", timeout: &durationpb.Duration{}, code: codes.InvalidArgument}, + {name: "negative", timeout: durationpb.New(-time.Second), code: codes.InvalidArgument}, + {name: "invalid-nanoseconds", timeout: &durationpb.Duration{Nanos: 1000000000}, code: codes.InvalidArgument}, + {name: "invalid-seconds", timeout: &durationpb.Duration{Seconds: 315576000001}, code: codes.InvalidArgument}, + } { + t.Run(tc.name, func(t *testing.T) { + source, err := tokenSource(tc.timeout, "http://127.0.0.1:1/token") + if got := status.Code(err); got != tc.code { + t.Fatalf("status = %s, want %s: %v", got, tc.code, err) + } + if (source != nil) != (tc.code == codes.OK) { + t.Fatal("invalid configuration returned a token source") + } + }) + } +} + +func TestTokenRequestTimeoutJSONConfiguration(t *testing.T) { + var config configuration.OAuth2Configuration + if err := protojson.Unmarshal([]byte(`{ + "scopes": ["example-scope"], + "clientCredentials": { + "clientId": "synthetic-client", + "clientSecret": "synthetic-secret", + "tokenEndpointUrl": "http://127.0.0.1:1/token", + "tokenRequestTimeout": "15s" + } + }`), &config); err != nil { + t.Fatal(err) + } + if got := config.GetClientCredentials().GetTokenRequestTimeout().AsDuration(); got != 15*time.Second { + t.Fatalf("JSON token request timeout = %s, want 15s", got) + } + if _, err := httpclient.NewOAuth2TokenSourceFromConfiguration(&config); err != nil { + t.Fatal(err) + } +} + +func TestTokenRequestTimeoutUnset(t *testing.T) { + t.Parallel() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Longer than the configured bound in the stall tests. An omitted + // setting must not impose that bound on existing configurations. + time.Sleep(400 * time.Millisecond) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "synthetic-unbounded", "token_type": "Bearer", "expires_in": 300, + }) + })) + defer server.Close() + source, err := tokenSource(nil, server.URL) + if err != nil { + t.Fatal(err) + } + token, err := source.Token() + if err != nil || token == nil || token.AccessToken != "synthetic-unbounded" { + t.Fatalf("unset timeout changed existing behavior: %v", err) + } +} + +func TestTokenRequestTimeoutFailsClosedThenRecovers(t *testing.T) { + for _, mode := range []string{"no-headers", "partial-body", "trickle-body"} { + t.Run(mode, func(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + var once sync.Once + unblock := func() { once.Do(func() { close(release) }) } + var recovering atomic.Bool + var calls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.ParseForm() != nil || r.Form.Get("grant_type") != "client_credentials" { + w.WriteHeader(http.StatusBadRequest) + return + } + calls.Add(1) + w.Header().Set("Content-Type", "application/json") + if recovering.Load() { + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "synthetic-recovered", "token_type": "Bearer", "expires_in": 300, + }) + return + } + if mode != "no-headers" { + _, _ = w.Write([]byte(`{"access_token":"`)) + w.(http.Flusher).Flush() + } + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + select { + case <-release: + return + case <-r.Context().Done(): + return + case <-ticker.C: + if mode == "trickle-body" { + if _, err := w.Write([]byte("x")); err != nil { + return + } + w.(http.Flusher).Flush() + } + } + } + })) + defer server.Close() + defer unblock() + source, err := tokenSource(durationpb.New(200*time.Millisecond), server.URL) + if err != nil { + t.Fatal(err) + } + type result struct { + token *oauth2.Token + err error + } + done := make(chan result, 1) + go func() { + token, err := source.Token() + done <- result{token, err} + }() + // Initial auth-style probing can make two HTTP requests. The + // watchdog only fails the test; it cannot establish a pass. + select { + case got := <-done: + // x/oauth2 formats body-read errors with %v, losing their + // error chain. Accept only its explicit Client.Timeout + // body marker when the deadline type is no longer visible. + timedOut := got.err != nil && (errors.Is(got.err, context.DeadlineExceeded) || + strings.Contains(got.err.Error(), "Client.Timeout or context cancellation while reading body")) + if got.token != nil || !timedOut { + t.Fatalf("stalled acquisition = %v, %v; want no token and HTTP timeout", got.token, got.err) + } + case <-time.After(5 * time.Second): + unblock() + <-done + t.Fatal("token acquisition exceeded the configured HTTP request bounds") + } + if got := calls.Load(); got < 1 || got > 2 { + t.Fatalf("acquisition requests = %d, want one or two", got) + } + recovering.Store(true) + token, err := source.Token() + if err != nil || token == nil || token.AccessToken != "synthetic-recovered" { + t.Fatalf("token source did not recover: %v", err) + } + before := calls.Load() + if _, err := source.Token(); err != nil || calls.Load() != before { + t.Fatal("valid recovered token was not reused") + } + }) + } +} diff --git a/pkg/proto/configuration/http/client/BUILD.bazel b/pkg/proto/configuration/http/client/BUILD.bazel index 4e6f1f60f..1f3310a2c 100644 --- a/pkg/proto/configuration/http/client/BUILD.bazel +++ b/pkg/proto/configuration/http/client/BUILD.bazel @@ -9,6 +9,7 @@ proto_library( visibility = ["//visibility:public"], deps = [ "//pkg/proto/configuration/tls:tls_proto", + "@protobuf//:duration_proto", "@protobuf//:empty_proto", ], ) diff --git a/pkg/proto/configuration/http/client/client.pb.go b/pkg/proto/configuration/http/client/client.pb.go index 56c74e828..318deb650 100644 --- a/pkg/proto/configuration/http/client/client.pb.go +++ b/pkg/proto/configuration/http/client/client.pb.go @@ -10,6 +10,7 @@ import ( tls "github.com/buildbarn/bb-storage/pkg/proto/configuration/tls" protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoimpl "google.golang.org/protobuf/runtime/protoimpl" + durationpb "google.golang.org/protobuf/types/known/durationpb" emptypb "google.golang.org/protobuf/types/known/emptypb" reflect "reflect" sync "sync" @@ -244,13 +245,14 @@ func (*OAuth2Configuration_ServiceAccountKey) isOAuth2Configuration_Credentials( func (*OAuth2Configuration_ClientCredentials) isOAuth2Configuration_Credentials() {} type OAuth2ClientCredentials struct { - state protoimpl.MessageState `protogen:"open.v1"` - ClientId string `protobuf:"bytes,1,opt,name=client_id,json=clientId,proto3" json:"client_id,omitempty"` - ClientSecret string `protobuf:"bytes,2,opt,name=client_secret,json=clientSecret,proto3" json:"client_secret,omitempty"` - TokenEndpointUrl string `protobuf:"bytes,3,opt,name=token_endpoint_url,json=tokenEndpointUrl,proto3" json:"token_endpoint_url,omitempty"` - HttpClient *Configuration `protobuf:"bytes,4,opt,name=http_client,json=httpClient,proto3" json:"http_client,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + ClientId string `protobuf:"bytes,1,opt,name=client_id,json=clientId,proto3" json:"client_id,omitempty"` + ClientSecret string `protobuf:"bytes,2,opt,name=client_secret,json=clientSecret,proto3" json:"client_secret,omitempty"` + TokenEndpointUrl string `protobuf:"bytes,3,opt,name=token_endpoint_url,json=tokenEndpointUrl,proto3" json:"token_endpoint_url,omitempty"` + HttpClient *Configuration `protobuf:"bytes,4,opt,name=http_client,json=httpClient,proto3" json:"http_client,omitempty"` + TokenRequestTimeout *durationpb.Duration `protobuf:"bytes,5,opt,name=token_request_timeout,json=tokenRequestTimeout,proto3" json:"token_request_timeout,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *OAuth2ClientCredentials) Reset() { @@ -311,6 +313,13 @@ func (x *OAuth2ClientCredentials) GetHttpClient() *Configuration { return nil } +func (x *OAuth2ClientCredentials) GetTokenRequestTimeout() *durationpb.Duration { + if x != nil { + return x.TokenRequestTimeout + } + return nil +} + type Configuration_HeaderValues struct { state protoimpl.MessageState `protogen:"open.v1"` Header string `protobuf:"bytes,1,opt,name=header,proto3" json:"header,omitempty"` @@ -367,7 +376,7 @@ var File_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_cli const file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_client_proto_rawDesc = "" + "\n" + - "Pgithub.com/buildbarn/bb-storage/pkg/proto/configuration/http/client/client.proto\x12#buildbarn.configuration.http.client\x1aEgithub.com/buildbarn/bb-storage/pkg/proto/configuration/tls/tls.proto\x1a\x1bgoogle/protobuf/empty.proto\"\xe4\x03\n" + + "Pgithub.com/buildbarn/bb-storage/pkg/proto/configuration/http/client/client.proto\x12#buildbarn.configuration.http.client\x1aEgithub.com/buildbarn/bb-storage/pkg/proto/configuration/tls/tls.proto\x1a\x1egoogle/protobuf/duration.proto\x1a\x1bgoogle/protobuf/empty.proto\"\xe4\x03\n" + "\rConfiguration\x12B\n" + "\x03tls\x18\x01 \x01(\v20.buildbarn.configuration.tls.ClientConfigurationR\x03tls\x12\x1d\n" + "\tproxy_url\x18\x02 \x01(\tH\x00R\bproxyUrl\x12N\n" + @@ -385,13 +394,14 @@ const file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_c "\x13service_account_key\x18\x02 \x01(\tH\x00R\x11serviceAccountKey\x12m\n" + "\x12client_credentials\x18\x04 \x01(\v2<.buildbarn.configuration.http.client.OAuth2ClientCredentialsH\x00R\x11clientCredentials\x12\x16\n" + "\x06scopes\x18\x03 \x03(\tR\x06scopesB\r\n" + - "\vcredentials\"\xde\x01\n" + + "\vcredentials\"\xad\x02\n" + "\x17OAuth2ClientCredentials\x12\x1b\n" + "\tclient_id\x18\x01 \x01(\tR\bclientId\x12#\n" + "\rclient_secret\x18\x02 \x01(\tR\fclientSecret\x12,\n" + "\x12token_endpoint_url\x18\x03 \x01(\tR\x10tokenEndpointUrl\x12S\n" + "\vhttp_client\x18\x04 \x01(\v22.buildbarn.configuration.http.client.ConfigurationR\n" + - "httpClientBEZCgithub.com/buildbarn/bb-storage/pkg/proto/configuration/http/clientb\x06proto3" + "httpClient\x12M\n" + + "\x15token_request_timeout\x18\x05 \x01(\v2\x19.google.protobuf.DurationR\x13tokenRequestTimeoutBEZCgithub.com/buildbarn/bb-storage/pkg/proto/configuration/http/clientb\x06proto3" var ( file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_client_proto_rawDescOnce sync.Once @@ -413,6 +423,7 @@ var file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_cli (*Configuration_HeaderValues)(nil), // 3: buildbarn.configuration.http.client.Configuration.HeaderValues (*tls.ClientConfiguration)(nil), // 4: buildbarn.configuration.tls.ClientConfiguration (*emptypb.Empty)(nil), // 5: google.protobuf.Empty + (*durationpb.Duration)(nil), // 6: google.protobuf.Duration } var file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_client_proto_depIdxs = []int32{ 4, // 0: buildbarn.configuration.http.client.Configuration.tls:type_name -> buildbarn.configuration.tls.ClientConfiguration @@ -422,11 +433,12 @@ var file_github_com_buildbarn_bb_storage_pkg_proto_configuration_http_client_cli 5, // 4: buildbarn.configuration.http.client.OAuth2Configuration.google_default_credentials:type_name -> google.protobuf.Empty 2, // 5: buildbarn.configuration.http.client.OAuth2Configuration.client_credentials:type_name -> buildbarn.configuration.http.client.OAuth2ClientCredentials 0, // 6: buildbarn.configuration.http.client.OAuth2ClientCredentials.http_client:type_name -> buildbarn.configuration.http.client.Configuration - 7, // [7:7] is the sub-list for method output_type - 7, // [7:7] is the sub-list for method input_type - 7, // [7:7] is the sub-list for extension type_name - 7, // [7:7] is the sub-list for extension extendee - 0, // [0:7] is the sub-list for field type_name + 6, // 7: buildbarn.configuration.http.client.OAuth2ClientCredentials.token_request_timeout:type_name -> google.protobuf.Duration + 8, // [8:8] is the sub-list for method output_type + 8, // [8:8] is the sub-list for method input_type + 8, // [8:8] is the sub-list for extension type_name + 8, // [8:8] is the sub-list for extension extendee + 0, // [0:8] is the sub-list for field type_name } func init() { diff --git a/pkg/proto/configuration/http/client/client.proto b/pkg/proto/configuration/http/client/client.proto index 12ff70861..a9b7f87e1 100644 --- a/pkg/proto/configuration/http/client/client.proto +++ b/pkg/proto/configuration/http/client/client.proto @@ -3,6 +3,7 @@ syntax = "proto3"; package buildbarn.configuration.http.client; import "github.com/buildbarn/bb-storage/pkg/proto/configuration/tls/tls.proto"; +import "google/protobuf/duration.proto"; import "google/protobuf/empty.proto"; option go_package = "github.com/buildbarn/bb-storage/pkg/proto/configuration/http/client"; @@ -88,4 +89,14 @@ message OAuth2ClientCredentials { // Configuration options for the HTTP client that is used to send // requests to the token endpoint and user info endpoint. Configuration http_client = 4; + + // Timeout for an entire HTTP request to the token endpoint, including + // connecting, redirects, waiting for headers, and reading the response + // body. When set, this duration must be positive. When unset, no timeout + // is imposed, preserving the behavior of existing configurations. + // + // The client may probe multiple authentication styles when obtaining + // its first token. Each request gets its own timeout. This does not + // bound time spent waiting for another token acquisition to finish. + google.protobuf.Duration token_request_timeout = 5; }