Skip to content

Commit f8ee52c

Browse files
committed
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.
1 parent c334a57 commit f8ee52c

File tree

10 files changed

+101
-43
lines changed

10 files changed

+101
-43
lines changed

go.work.sum

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2043,6 +2043,7 @@ golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY=
20432043
golang.org/x/net v0.23.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
20442044
golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM=
20452045
golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE=
2046+
golang.org/x/net v0.28.0/go.mod h1:yqtgsTWOOnlGLG9GFRrK3++bGOUEkNBoHZc8MEDWPNg=
20462047
golang.org/x/net v0.30.0/go.mod h1:2wGyMJ5iFasEhkwi13ChkO/t1ECNC4X4eBKkVFyYFlU=
20472048
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
20482049
golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
@@ -2192,6 +2193,7 @@ golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21922193
golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21932194
golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21942195
golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
2196+
golang.org/x/sys v0.23.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21952197
golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21962198
golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
21972199
golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 h1:+Kc94D8UVEVxJnLXp/+FMfqQARZtWHfVrcRtcG8aT3g=
@@ -2225,6 +2227,7 @@ golang.org/x/text v0.12.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
22252227
golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
22262228
golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
22272229
golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI=
2230+
golang.org/x/text v0.17.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY=
22282231
golang.org/x/text v0.19.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY=
22292232
golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
22302233
golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=

sdk/auth/oauth/oauth.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package oauth
22

33
import (
44
"context"
5-
"crypto/tls"
65
"encoding/json"
76
"fmt"
87
"io"
@@ -24,8 +23,8 @@ const (
2423
)
2524

2625
type CertExchangeInfo struct {
27-
TLSConfig *tls.Config
28-
Audience []string
26+
HTTPClient *http.Client
27+
Audience []string
2928
}
3029

3130
type ClientCredentials struct {
@@ -313,13 +312,8 @@ func DoCertExchange(ctx context.Context, tokenEndpoint string, exchangeInfo Cert
313312
if err != nil {
314313
return nil, err
315314
}
316-
client := &http.Client{
317-
Transport: &http.Transport{
318-
TLSClientConfig: exchangeInfo.TLSConfig,
319-
},
320-
}
321315

322-
resp, err := client.Do(req)
316+
resp, err := exchangeInfo.HTTPClient.Do(req)
323317
if err != nil {
324318
return nil, fmt.Errorf("error making request to IdP for certificate exchange: %w", err)
325319
}

sdk/auth/oauth/oauth_test.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/lestrrat-go/jwx/v2/jws"
2424
"github.com/lestrrat-go/jwx/v2/jwt"
2525
"github.com/opentdf/platform/lib/fixtures"
26+
"github.com/opentdf/platform/sdk/httputil"
2627
"github.com/stretchr/testify/require"
2728
"github.com/stretchr/testify/suite"
2829
tc "github.com/testcontainers/testcontainers-go"
@@ -78,12 +79,15 @@ func (s *OAuthSuite) TestCertExchangeFromKeycloak() {
7879
rootCAs, _ := x509.SystemCertPool()
7980
rootCAs.AppendCertsFromPEM(ca)
8081
s.Require().NoError(err)
81-
tlsConfig := tls.Config{
82+
tlsConfig := &tls.Config{
8283
MinVersion: tls.VersionTLS12,
8384
Certificates: []tls.Certificate{cert},
8485
RootCAs: rootCAs,
8586
}
86-
exhcangeInfo := CertExchangeInfo{TLSConfig: &tlsConfig, Audience: []string{"opentdf-sdk"}}
87+
exhcangeInfo := CertExchangeInfo{
88+
HTTPClient: httputil.SafeHTTPClientWithTLSConfig(tlsConfig),
89+
Audience: []string{"opentdf-sdk"},
90+
}
8791

8892
tok, err := DoCertExchange(
8993
context.Background(),

sdk/auth/token_adding_interceptor.go

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"crypto/rand"
66
"crypto/sha256"
7-
"crypto/tls"
87
"encoding/base64"
98
"fmt"
109
"log/slog"
@@ -24,16 +23,16 @@ const (
2423
JTILength = 14
2524
)
2625

27-
func NewTokenAddingInterceptor(t AccessTokenSource, c *tls.Config) TokenAddingInterceptor {
26+
func NewTokenAddingInterceptor(t AccessTokenSource, c *http.Client) TokenAddingInterceptor {
2827
return TokenAddingInterceptor{
2928
tokenSource: t,
30-
tlsConfig: c,
29+
httpClient: c,
3130
}
3231
}
3332

3433
type TokenAddingInterceptor struct {
3534
tokenSource AccessTokenSource
36-
tlsConfig *tls.Config
35+
httpClient *http.Client
3736
}
3837

3938
func (i TokenAddingInterceptor) AddCredentials(
@@ -45,12 +44,7 @@ func (i TokenAddingInterceptor) AddCredentials(
4544
opts ...grpc.CallOption,
4645
) error {
4746
newMetadata := make([]string, 0)
48-
client := &http.Client{
49-
Transport: &http.Transport{
50-
TLSClientConfig: i.tlsConfig,
51-
},
52-
}
53-
accessToken, err := i.tokenSource.AccessToken(ctx, client)
47+
accessToken, err := i.tokenSource.AccessToken(ctx, i.httpClient)
5448
if err == nil {
5549
newMetadata = append(newMetadata, "Authorization", fmt.Sprintf("DPoP %s", accessToken))
5650
} else {

sdk/auth/token_adding_interceptor_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/lestrrat-go/jwx/v2/jws"
1919
"github.com/lestrrat-go/jwx/v2/jwt"
2020
"github.com/opentdf/platform/protocol/go/kas"
21+
"github.com/opentdf/platform/sdk/httputil"
2122
"github.com/stretchr/testify/assert"
2223
"github.com/stretchr/testify/require"
2324
"google.golang.org/grpc"
@@ -42,9 +43,9 @@ func TestAddingTokensToOutgoingRequest(t *testing.T) {
4243
accessToken: "thisisafakeaccesstoken",
4344
}
4445
server := FakeAccessServiceServer{}
45-
oo := NewTokenAddingInterceptor(&ts, &tls.Config{
46+
oo := NewTokenAddingInterceptor(&ts, httputil.SafeHTTPClientWithTLSConfig(&tls.Config{
4647
MinVersion: tls.VersionTLS12,
47-
})
48+
}))
4849

4950
client, stop := runServer(&server, oo)
5051
defer stop()
@@ -97,9 +98,9 @@ func TestAddingTokensToOutgoingRequest(t *testing.T) {
9798
func Test_InvalidCredentials_DoesNotSendMessage(t *testing.T) {
9899
ts := FakeTokenSource{key: nil, accessToken: ""}
99100
server := FakeAccessServiceServer{}
100-
oo := NewTokenAddingInterceptor(&ts, &tls.Config{
101+
oo := NewTokenAddingInterceptor(&ts, httputil.SafeHTTPClientWithTLSConfig(&tls.Config{
101102
MinVersion: tls.VersionTLS12,
102-
})
103+
}))
103104

104105
client, stop := runServer(&server, oo)
105106
defer stop()

sdk/httputil/http.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
package httputil
2+
3+
import (
4+
"crypto/tls"
5+
"net/http"
6+
"time"
7+
)
8+
9+
const (
10+
defaultTimeout = 120 * time.Second
11+
// defaults to match DefaultTransport - defined to satisfy lint
12+
maxIdleConns = 100
13+
idleConnTimeout = 90 * time.Second
14+
tLSHandshakeTimeout = 10 * time.Second
15+
expectContinueTimeout = 1 * time.Second
16+
)
17+
18+
var preventRedirectCheck = func(_ *http.Request, _ []*http.Request) error {
19+
return http.ErrUseLastResponse // Prevent following redirects
20+
}
21+
22+
var safeHTTPClient = &http.Client{
23+
Transport: http.DefaultTransport,
24+
Timeout: defaultTimeout,
25+
CheckRedirect: preventRedirectCheck,
26+
}
27+
28+
// SafeHTTPClient returns a default http client which has sensible timeouts, won't follow redirects, and enables idle
29+
// connection pooling.
30+
func SafeHTTPClient() *http.Client {
31+
return safeHTTPClient
32+
}
33+
34+
// SafeHTTPClientWithTLSConfig returns a http client which has sensible timeouts, won't follow redirects, and if
35+
// specified a http.Transport with the tls.Config provided.
36+
func SafeHTTPClientWithTLSConfig(cfg *tls.Config) *http.Client {
37+
if cfg == nil {
38+
return safeHTTPClient
39+
}
40+
return SafeHTTPClientWithTransport(&http.Transport{
41+
TLSClientConfig: cfg,
42+
// config below matches DefaultTransport
43+
Proxy: http.ProxyFromEnvironment,
44+
ForceAttemptHTTP2: true,
45+
MaxIdleConns: maxIdleConns,
46+
IdleConnTimeout: idleConnTimeout,
47+
TLSHandshakeTimeout: tLSHandshakeTimeout,
48+
ExpectContinueTimeout: expectContinueTimeout,
49+
})
50+
}
51+
52+
// SafeHTTPClientWithTransport returns a http client which has sensible timeouts, won't follow redirects, and if
53+
// specified the provided http.Transport.
54+
func SafeHTTPClientWithTransport(transport *http.Transport) *http.Client {
55+
if transport == nil {
56+
return safeHTTPClient
57+
}
58+
return &http.Client{
59+
Transport: transport,
60+
// config below matches our values for safeHttpClient
61+
Timeout: defaultTimeout,
62+
CheckRedirect: preventRedirectCheck,
63+
}
64+
}

sdk/options.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@ package sdk
33
import (
44
"crypto/rsa"
55
"crypto/tls"
6+
"net/http"
67

78
"github.com/opentdf/platform/lib/ocrypto"
89
"github.com/opentdf/platform/sdk/auth"
910
"github.com/opentdf/platform/sdk/auth/oauth"
11+
"github.com/opentdf/platform/sdk/httputil"
1012
"golang.org/x/oauth2"
1113
"google.golang.org/grpc"
1214
"google.golang.org/grpc/credentials"
@@ -20,7 +22,7 @@ type config struct {
2022
// Platform configuration structure is subject to change. Consume via accessor methods.
2123
PlatformConfiguration PlatformConfiguration
2224
dialOption grpc.DialOption
23-
tlsConfig *tls.Config
25+
httpClient *http.Client
2426
clientCredentials *oauth.ClientCredentials
2527
tokenExchange *oauth.TokenExchangeInfo
2628
tokenEndpoint string
@@ -66,7 +68,7 @@ func WithInsecureSkipVerifyConn() Option {
6668
}
6769
c.dialOption = grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))
6870
// used by http client
69-
c.tlsConfig = tlsConfig
71+
c.httpClient = httputil.SafeHTTPClientWithTLSConfig(tlsConfig)
7072
}
7173
}
7274

@@ -83,7 +85,7 @@ func WithInsecurePlaintextConn() Option {
8385
c.dialOption = grpc.WithTransportCredentials(insecure.NewCredentials())
8486
// used by http client
8587
// FIXME anything to do here
86-
c.tlsConfig = &tls.Config{}
88+
c.httpClient = httputil.SafeHTTPClient()
8789
}
8890
}
8991

@@ -97,7 +99,7 @@ func WithClientCredentials(clientID, clientSecret string, scopes []string) Optio
9799

98100
func WithTLSCredentials(tls *tls.Config, audience []string) Option {
99101
return func(c *config) {
100-
c.certExchange = &oauth.CertExchangeInfo{TLSConfig: tls, Audience: audience}
102+
c.certExchange = &oauth.CertExchangeInfo{HTTPClient: httputil.SafeHTTPClientWithTLSConfig(tls), Audience: audience}
101103
}
102104
}
103105

sdk/sdk.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func New(platformEndpoint string, opts ...Option) (*SDK, error) {
154154
return nil, err
155155
}
156156
if accessTokenSource != nil {
157-
interceptor := auth.NewTokenAddingInterceptor(accessTokenSource, cfg.tlsConfig)
157+
interceptor := auth.NewTokenAddingInterceptor(accessTokenSource, cfg.httpClient)
158158
uci = append(uci, interceptor.AddCredentials)
159159
}
160160

@@ -452,13 +452,7 @@ func getTokenEndpoint(c config) (string, error) {
452452
return "", err
453453
}
454454

455-
httpClient := &http.Client{
456-
Transport: &http.Transport{
457-
TLSClientConfig: c.tlsConfig,
458-
},
459-
}
460-
461-
resp, err := httpClient.Do(req)
455+
resp, err := c.httpClient.Do(req)
462456
if err != nil {
463457
return "", err
464458
}

service/internal/auth/authn_test.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"github.com/opentdf/platform/protocol/go/kas"
3030
"github.com/opentdf/platform/protocol/go/kas/kasconnect"
3131
sdkauth "github.com/opentdf/platform/sdk/auth"
32+
"github.com/opentdf/platform/sdk/httputil"
3233
"github.com/opentdf/platform/service/internal/server/memhttp"
3334
"github.com/opentdf/platform/service/logger"
3435
ctxAuth "github.com/opentdf/platform/service/pkg/auth"
@@ -461,9 +462,9 @@ func (s *AuthSuite) TestDPoPEndToEnd_GRPC() {
461462
addingInterceptor := sdkauth.NewTokenAddingInterceptor(&FakeTokenSource{
462463
key: dpopKey,
463464
accessToken: string(signedTok),
464-
}, &tls.Config{
465+
}, httputil.SafeHTTPClientWithTLSConfig(&tls.Config{
465466
MinVersion: tls.VersionTLS12,
466-
})
467+
}))
467468

468469
conn, _ := grpc.NewClient("passthrough://bufconn", grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) {
469470
return server.Listener.DialContext(ctx, "tcp", "http://localhost:8080")
@@ -522,16 +523,16 @@ func (s *AuthSuite) TestDPoPEndToEnd_HTTP() {
522523
addingInterceptor := sdkauth.NewTokenAddingInterceptor(&FakeTokenSource{
523524
key: dpopKey,
524525
accessToken: string(signedTok),
525-
}, &tls.Config{
526+
}, httputil.SafeHTTPClientWithTLSConfig(&tls.Config{
526527
MinVersion: tls.VersionTLS12,
527-
})
528+
}))
528529
s.Require().NoError(err)
529530
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", signedTok))
530531
dpopTok, err := addingInterceptor.GetDPoPToken(server.URL+"/attributes", "GET", string(signedTok))
531532
s.Require().NoError(err)
532533
req.Header.Set("DPoP", dpopTok)
533534

534-
client := http.Client{}
535+
client := httputil.SafeHTTPClient() // use safe client to help validate the client
535536
_, err = client.Do(req)
536537
s.Require().NoError(err)
537538
var dpopKeyFromRequest jwk.Key

service/pkg/server/start.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/opentdf/platform/sdk"
1717
sdkauth "github.com/opentdf/platform/sdk/auth"
1818
"github.com/opentdf/platform/sdk/auth/oauth"
19+
"github.com/opentdf/platform/sdk/httputil"
1920
"github.com/opentdf/platform/service/internal/auth"
2021
"github.com/opentdf/platform/service/internal/config"
2122
"github.com/opentdf/platform/service/internal/server"
@@ -231,7 +232,7 @@ func Start(f ...StartOptions) error {
231232
return fmt.Errorf("error creating ERS tokensource: %w", err)
232233
}
233234

234-
interceptor := sdkauth.NewTokenAddingInterceptor(ts, tlsConfig)
235+
interceptor := sdkauth.NewTokenAddingInterceptor(ts, httputil.SafeHTTPClientWithTLSConfig(tlsConfig))
235236

236237
ersDialOptions = append(ersDialOptions, grpc.WithChainUnaryInterceptor(interceptor.AddCredentials))
237238
}

0 commit comments

Comments
 (0)