fix: Improve http.Client usage for security and performance

This change switches us from maintaining a tls config which we then on-demand initialize an `http.Client` with to instead maintain and reuse an `http.Client` instance.
This enables us to utilize the connection pooling which occurs within the `http.Transport` to reduce ssl handshakes and thus reduce latency.

In addition this change provides us a central place to configure out `http.Client` (`httputil`). Allowing us to easily set configuration options to reduce the security risks of using an unconfigured `http.Client`. Notably timeouts to reduce DoS risks, and control around following redirects to prevent blind SSRF's.

This change will provide significant benefit without making API changes.  A future PR will update the service and also cleanup some of the API.

Author: jentfoo

sdk/auth/oauth/oauth.go

@@ -2,7 +2,6 @@ package oauth
 
 import (
 	"context"
-	"crypto/tls"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -17,15 +16,16 @@ import (
 	"github.com/lestrrat-go/jwx/v2/jwk"
 	"github.com/lestrrat-go/jwx/v2/jws"
 	"github.com/lestrrat-go/jwx/v2/jwt"
+	"github.com/opentdf/platform/sdk/httputil"
 )
 
 const (
 	tokenExpirationBuffer = 10 * time.Second
 )
 
 type CertExchangeInfo struct {
-	TLSConfig *tls.Config
-	Audience  []string
+	HTTPClient *http.Client
+	Audience   []string
 }
 
 type ClientCredentials struct {
@@ -146,6 +146,9 @@ func GetAccessToken(client *http.Client, tokenEndpoint string, scopes []string,
 	if err != nil {
 		return nil, err
 	}
+	if client == nil {
+		client = httputil.SafeHTTPClient()
+	}
 
 	resp, err := client.Do(req)
 	if err != nil {
@@ -249,6 +252,9 @@ func DoTokenExchange(ctx context.Context, client *http.Client, tokenEndpoint str
 	if err != nil {
 		return nil, err
 	}
+	if client == nil {
+		client = httputil.SafeHTTPClient()
+	}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("error making request to IdP for token exchange: %w", err)
@@ -313,12 +319,11 @@ func DoCertExchange(ctx context.Context, tokenEndpoint string, exchangeInfo Cert
 	if err != nil {
 		return nil, err
 	}
-	client := &http.Client{
-		Transport: &http.Transport{
-			TLSClientConfig: exchangeInfo.TLSConfig,
-		},
-	}
 
+	client := exchangeInfo.HTTPClient
+	if client == nil {
+		client = httputil.SafeHTTPClient()
+	}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("error making request to IdP for certificate exchange: %w", err)

sdk/auth/oauth/oauth_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/lestrrat-go/jwx/v2/jws"
 	"github.com/lestrrat-go/jwx/v2/jwt"
 	"github.com/opentdf/platform/lib/fixtures"
+	"github.com/opentdf/platform/sdk/httputil"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 	tc "github.com/testcontainers/testcontainers-go"
@@ -78,12 +79,15 @@ func (s *OAuthSuite) TestCertExchangeFromKeycloak() {
 	rootCAs, _ := x509.SystemCertPool()
 	rootCAs.AppendCertsFromPEM(ca)
 	s.Require().NoError(err)
-	tlsConfig := tls.Config{
+	tlsConfig := &tls.Config{
 		MinVersion:   tls.VersionTLS12,
 		Certificates: []tls.Certificate{cert},
 		RootCAs:      rootCAs,
 	}
-	exhcangeInfo := CertExchangeInfo{TLSConfig: &tlsConfig, Audience: []string{"opentdf-sdk"}}
+	exhcangeInfo := CertExchangeInfo{
+		HTTPClient: httputil.SafeHTTPClientWithTLSConfig(tlsConfig),
+		Audience:   []string{"opentdf-sdk"},
+	}
 
 	tok, err := DoCertExchange(
 		context.Background(),

sdk/auth/token_adding_interceptor.go

@@ -14,6 +14,7 @@ import (
 	"github.com/lestrrat-go/jwx/v2/jwk"
 	"github.com/lestrrat-go/jwx/v2/jws"
 	"github.com/lestrrat-go/jwx/v2/jwt"
+	"github.com/opentdf/platform/sdk/httputil"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
@@ -25,15 +26,22 @@ const (
 )
 
 func NewTokenAddingInterceptor(t AccessTokenSource, c *tls.Config) TokenAddingInterceptor {
+	return NewTokenAddingInterceptorWithClient(t, httputil.SafeHTTPClientWithTLSConfig(c))
+}
+
+func NewTokenAddingInterceptorWithClient(t AccessTokenSource, c *http.Client) TokenAddingInterceptor {
+	if c == nil {
+		c = httputil.SafeHTTPClient()
+	}
 	return TokenAddingInterceptor{
 		tokenSource: t,
-		tlsConfig:   c,
+		httpClient:  c,
 	}
 }
 
 type TokenAddingInterceptor struct {
 	tokenSource AccessTokenSource
-	tlsConfig   *tls.Config
+	httpClient  *http.Client
 }
 
 func (i TokenAddingInterceptor) AddCredentials(
@@ -45,12 +53,7 @@ func (i TokenAddingInterceptor) AddCredentials(
 	opts ...grpc.CallOption,
 ) error {
 	newMetadata := make([]string, 0)
-	client := &http.Client{
-		Transport: &http.Transport{
-			TLSClientConfig: i.tlsConfig,
-		},
-	}
-	accessToken, err := i.tokenSource.AccessToken(ctx, client)
+	accessToken, err := i.tokenSource.AccessToken(ctx, i.httpClient)
 	if err == nil {
 		newMetadata = append(newMetadata, "Authorization", fmt.Sprintf("DPoP %s", accessToken))
 	} else {

sdk/httputil/http.go

@@ -0,0 +1,64 @@
+package httputil
+
+import (
+	"crypto/tls"
+	"net/http"
+	"time"
+)
+
+const (
+	defaultTimeout = 120 * time.Second
+	// defaults to match DefaultTransport - defined to satisfy lint
+	maxIdleConns          = 100
+	idleConnTimeout       = 90 * time.Second
+	tLSHandshakeTimeout   = 10 * time.Second
+	expectContinueTimeout = 1 * time.Second
+)
+
+var preventRedirectCheck = func(_ *http.Request, _ []*http.Request) error {
+	return http.ErrUseLastResponse // Prevent following redirects
+}
+
+var safeHTTPClient = &http.Client{
+	Transport:     http.DefaultTransport,
+	Timeout:       defaultTimeout,
+	CheckRedirect: preventRedirectCheck,
+}
+
+// SafeHTTPClient returns a default http client which has sensible timeouts, won't follow redirects, and enables idle
+// connection pooling.
+func SafeHTTPClient() *http.Client {
+	return safeHTTPClient
+}
+
+// SafeHTTPClientWithTLSConfig returns a http client which has sensible timeouts, won't follow redirects, and if
+// specified a http.Transport with the tls.Config provided.
+func SafeHTTPClientWithTLSConfig(cfg *tls.Config) *http.Client {
+	if cfg == nil {
+		return safeHTTPClient
+	}
+	return SafeHTTPClientWithTransport(&http.Transport{
+		TLSClientConfig: cfg,
+		// config below matches DefaultTransport
+		Proxy:                 http.ProxyFromEnvironment,
+		ForceAttemptHTTP2:     true,
+		MaxIdleConns:          maxIdleConns,
+		IdleConnTimeout:       idleConnTimeout,
+		TLSHandshakeTimeout:   tLSHandshakeTimeout,
+		ExpectContinueTimeout: expectContinueTimeout,
+	})
+}
+
+// SafeHTTPClientWithTransport returns a http client which has sensible timeouts, won't follow redirects, and if
+// specified the provided http.Transport.
+func SafeHTTPClientWithTransport(transport *http.Transport) *http.Client {
+	if transport == nil {
+		return safeHTTPClient
+	}
+	return &http.Client{
+		Transport: transport,
+		// config below matches our values for safeHttpClient
+		Timeout:       defaultTimeout,
+		CheckRedirect: preventRedirectCheck,
+	}
+}

sdk/options.go

@@ -3,10 +3,12 @@ package sdk
 import (
 	"crypto/rsa"
 	"crypto/tls"
+	"net/http"
 
 	"github.com/opentdf/platform/lib/ocrypto"
 	"github.com/opentdf/platform/sdk/auth"
 	"github.com/opentdf/platform/sdk/auth/oauth"
+	"github.com/opentdf/platform/sdk/httputil"
 	"golang.org/x/oauth2"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
@@ -20,7 +22,7 @@ type config struct {
 	// Platform configuration structure is subject to change. Consume via accessor methods.
 	PlatformConfiguration   PlatformConfiguration
 	dialOption              grpc.DialOption
-	tlsConfig               *tls.Config
+	httpClient              *http.Client
 	clientCredentials       *oauth.ClientCredentials
 	tokenExchange           *oauth.TokenExchangeInfo
 	tokenEndpoint           string
@@ -66,7 +68,7 @@ func WithInsecureSkipVerifyConn() Option {
 		}
 		c.dialOption = grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))
 		// used by http client
-		c.tlsConfig = tlsConfig
+		c.httpClient = httputil.SafeHTTPClientWithTLSConfig(tlsConfig)
 	}
 }
 
@@ -83,7 +85,7 @@ func WithInsecurePlaintextConn() Option {
 		c.dialOption = grpc.WithTransportCredentials(insecure.NewCredentials())
 		// used by http client
 		// FIXME anything to do here
-		c.tlsConfig = &tls.Config{}
+		c.httpClient = httputil.SafeHTTPClient()
 	}
 }
 
@@ -97,7 +99,7 @@ func WithClientCredentials(clientID, clientSecret string, scopes []string) Optio
 
 func WithTLSCredentials(tls *tls.Config, audience []string) Option {
 	return func(c *config) {
-		c.certExchange = &oauth.CertExchangeInfo{TLSConfig: tls, Audience: audience}
+		c.certExchange = &oauth.CertExchangeInfo{HTTPClient: httputil.SafeHTTPClientWithTLSConfig(tls), Audience: audience}
 	}
 }
 

sdk/sdk.go

@@ -27,6 +27,7 @@ import (
 	"github.com/opentdf/platform/protocol/go/wellknownconfiguration"
 	"github.com/opentdf/platform/sdk/audit"
 	"github.com/opentdf/platform/sdk/auth"
+	"github.com/opentdf/platform/sdk/httputil"
 	"github.com/opentdf/platform/sdk/internal/archive"
 	"github.com/xeipuuv/gojsonschema"
 	"google.golang.org/grpc"
@@ -154,7 +155,7 @@ func New(platformEndpoint string, opts ...Option) (*SDK, error) {
 		return nil, err
 	}
 	if accessTokenSource != nil {
-		interceptor := auth.NewTokenAddingInterceptor(accessTokenSource, cfg.tlsConfig)
+		interceptor := auth.NewTokenAddingInterceptorWithClient(accessTokenSource, cfg.httpClient)
 		uci = append(uci, interceptor.AddCredentials)
 	}
 
@@ -452,13 +453,11 @@ func getTokenEndpoint(c config) (string, error) {
 		return "", err
 	}
 
-	httpClient := &http.Client{
-		Transport: &http.Transport{
-			TLSClientConfig: c.tlsConfig,
-		},
+	client := c.httpClient
+	if client == nil {
+		client = httputil.SafeHTTPClient()
 	}
-
-	resp, err := httpClient.Do(req)
+	resp, err := client.Do(req)
 	if err != nil {
 		return "", err
 	}