Skip to content

Commit fa46906

Browse files
github-actions[bot]f0sselgeokat
authored
fix!: only trust x-forwarded-host from configured trusted proxies (#26204) (conflicts) (#26299)
Backport of #26204 Original PR: #26204 — fix!: only trust x-forwarded-host from configured trusted proxies Merge commit: b5ef700 Requested by: @f0ssel > [!WARNING] > The automatic cherry-pick had conflicts. > Please resolve manually by cherry-picking the original merge commit: > > ``` > git fetch origin backport/26204-to-2.33 > git checkout backport/26204-to-2.33 > git reset --hard origin/release/2.33 > git cherry-pick -x -m1 b5ef700 > # resolve conflicts, then push > ``` --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com> Co-authored-by: George K <george@coder.com>
1 parent 91d1865 commit fa46906

17 files changed

Lines changed: 290 additions & 30 deletions

File tree

‎agent/api.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func (a *agent) apiHandler() http.Handler {
1919
r.Use(
2020
httpmw.Recover(a.logger),
2121
tracing.StatusWriterMiddleware,
22-
loggermw.Logger(a.logger),
22+
loggermw.Logger(a.logger, nil),
2323
)
2424
r.Get("/", func(rw http.ResponseWriter, r *http.Request) {
2525
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{

‎cli/testdata/coder_server_--help.golden‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -403,8 +403,8 @@ NETWORKING OPTIONS:
403403
True-Client-Ip, X-Forwarded-For.
404404

405405
--proxy-trusted-origins string-array, $CODER_PROXY_TRUSTED_ORIGINS
406-
Origin addresses to respect "proxy-trusted-headers". e.g.
407-
192.168.1.0/24.
406+
Origin addresses to respect "proxy-trusted-headers" and
407+
X-Forwarded-Host for subdomain app routing. e.g. 192.168.1.0/24.
408408

409409
--redirect-to-access-url bool, $CODER_REDIRECT_TO_ACCESS_URL
410410
Specifies whether to redirect requests that do not match the access

‎cli/testdata/server-config.yaml.golden‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ networking:
172172
# True-Client-Ip, X-Forwarded-For.
173173
# (default: <unset>, type: string-array)
174174
proxyTrustedHeaders: []
175-
# Origin addresses to respect "proxy-trusted-headers". e.g. 192.168.1.0/24.
175+
# Origin addresses to respect "proxy-trusted-headers" and X-Forwarded-Host for
176+
# subdomain app routing. e.g. 192.168.1.0/24.
176177
# (default: <unset>, type: string-array)
177178
proxyTrustedOrigins: []
178179
# Controls if the 'Secure' property is set on browser session cookies.

‎coderd/coderd.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -962,7 +962,9 @@ func New(options *Options) *API {
962962
tracing.Middleware(api.TracerProvider),
963963
httpmw.AttachRequestID,
964964
httpmw.ExtractRealIP(api.RealIPConfig),
965-
loggermw.Logger(api.Logger),
965+
loggermw.Logger(api.Logger, func(r *http.Request) string {
966+
return httpmw.EffectiveHost(api.RealIPConfig, r)
967+
}),
966968
singleSlashMW,
967969
rolestore.CustomRoleMW,
968970
// Validate API key on every request (if present) and store

‎coderd/httpapi/request.go‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,6 @@ const (
88
XForwardedHostHeader = "X-Forwarded-Host"
99
)
1010

11-
// RequestHost returns the name of the host from the request. It prioritizes
12-
// 'X-Forwarded-Host' over r.Host since most requests are being proxied.
13-
func RequestHost(r *http.Request) string {
14-
host := r.Header.Get(XForwardedHostHeader)
15-
if host != "" {
16-
return host
17-
}
18-
19-
return r.Host
20-
}
21-
2211
func IsWebsocketUpgrade(r *http.Request) bool {
2312
vs := r.Header.Values("Upgrade")
2413
for _, v := range vs {

‎coderd/httpmw/loggermw/logger.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"github.com/go-chi/chi/v5"
1313

1414
"cdr.dev/slog/v3"
15-
"github.com/coder/coder/v2/coderd/httpapi"
1615
"github.com/coder/coder/v2/coderd/tracing"
1716
)
1817

@@ -69,7 +68,7 @@ func safeQueryParams(params url.Values) []slog.Field {
6968
return fields
7069
}
7170

72-
func Logger(log slog.Logger) func(next http.Handler) http.Handler {
71+
func Logger(log slog.Logger, hostResolver func(*http.Request) string) func(next http.Handler) http.Handler {
7372
return func(next http.Handler) http.Handler {
7473
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
7574
start := time.Now()
@@ -79,9 +78,15 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
7978
panic(fmt.Sprintf("ResponseWriter not a *tracing.StatusWriter; got %T", rw))
8079
}
8180

81+
host := r.Host
82+
if hostResolver != nil {
83+
host = hostResolver(r)
84+
}
85+
8286
httplog := log.With(
8387
slog.F("user_agent", r.Header.Get("User-Agent")),
84-
slog.F("host", httpapi.RequestHost(r)),
88+
slog.F("host", host),
89+
slog.F("received_host", r.Host),
8590
slog.F("path", r.URL.Path),
8691
slog.F("proto", r.Proto),
8792
slog.F("remote_addr", r.RemoteAddr),

‎coderd/httpmw/loggermw/logger_internal_test.go‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
6868
})
6969

7070
// Wrap the test handler with the Logger middleware
71-
loggerMiddleware := Logger(logger)
71+
loggerMiddleware := Logger(logger, nil)
7272
wrappedHandler := loggerMiddleware(testHandler)
7373

7474
// Create a test HTTP request
@@ -91,7 +91,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
9191
}
9292

9393
// Check that the log contains the expected fields
94-
requiredFields := []string{"host", "path", "proto", "remote_addr", "start", "took", "status_code", "user_agent", "latency_ms"}
94+
requiredFields := []string{"host", "received_host", "path", "proto", "remote_addr", "start", "took", "status_code", "user_agent", "latency_ms"}
9595
for _, field := range requiredFields {
9696
_, exists := fieldsMap[field]
9797
require.True(t, exists, "field %q is missing in log fields", field)
@@ -103,6 +103,38 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
103103
require.Equal(t, fieldsMap["status_code"], http.StatusOK)
104104
}
105105

106+
func TestLoggerMiddleware_HostFields(t *testing.T) {
107+
t.Parallel()
108+
109+
sink := testutil.NewFakeSink(t)
110+
logger := sink.Logger()
111+
112+
testHandler := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
113+
rw.WriteHeader(http.StatusOK)
114+
})
115+
116+
loggerMiddleware := Logger(logger, func(_ *http.Request) string {
117+
return "effective.test"
118+
})
119+
wrappedHandler := loggerMiddleware(testHandler)
120+
121+
req := httptest.NewRequest(http.MethodGet, "http://received.test/path", nil)
122+
123+
sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()}
124+
wrappedHandler.ServeHTTP(sw, req)
125+
126+
entries := sink.Entries()
127+
require.Len(t, entries, 1, "expected exactly one log entry")
128+
129+
fieldsMap := make(map[string]any)
130+
for _, field := range entries[0].Fields {
131+
fieldsMap[field.Name] = field.Value
132+
}
133+
134+
require.Equal(t, "effective.test", fieldsMap["host"])
135+
require.Equal(t, "received.test", fieldsMap["received_host"])
136+
}
137+
106138
func TestLoggerMiddleware_WebSocket(t *testing.T) {
107139
t.Parallel()
108140
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -129,7 +161,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) {
129161
})
130162

131163
// Wrap the test handler with the Logger middleware
132-
loggerMiddleware := Logger(logger)
164+
loggerMiddleware := Logger(logger, nil)
133165
wrappedHandler := loggerMiddleware(testHandler)
134166

135167
// RequestLogger expects the ResponseWriter to be *tracing.StatusWriter
@@ -186,7 +218,7 @@ func TestRequestLogger_HTTPRouteParams(t *testing.T) {
186218
})
187219

188220
// Wrap the test handler with the Logger middleware
189-
loggerMiddleware := Logger(logger)
221+
loggerMiddleware := Logger(logger, nil)
190222
wrappedHandler := loggerMiddleware(testHandler)
191223

192224
// Create a test HTTP request

‎coderd/httpmw/realip.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,35 @@ func FilterUntrustedOriginHeaders(config *RealIPConfig, req *http.Request) {
105105
}
106106
}
107107

108+
// EffectiveHost returns the host Coder should trust for request handling.
109+
// It uses X-Forwarded-Host only when the immediate peer is a configured
110+
// trusted proxy. Otherwise it uses the received Host header.
111+
func EffectiveHost(config *RealIPConfig, r *http.Request) string {
112+
if config == nil {
113+
config = &RealIPConfig{
114+
TrustedOrigins: nil,
115+
TrustedHeaders: nil,
116+
}
117+
}
118+
119+
// When ExtractRealIP has run, r.RemoteAddr may hold the forwarded
120+
// client IP, and we should use the original socket peer for proxy
121+
// trust decisions.
122+
remoteAddr := r.RemoteAddr
123+
state := RealIP(r.Context())
124+
if state != nil && state.OriginalRemoteAddr != "" {
125+
remoteAddr = state.OriginalRemoteAddr
126+
}
127+
128+
if isContainedIn(config.TrustedOrigins, getRemoteAddress(remoteAddr)) {
129+
if host := r.Header.Get(httpapi.XForwardedHostHeader); host != "" {
130+
return host
131+
}
132+
}
133+
134+
return r.Host
135+
}
136+
108137
// EnsureXForwardedForHeader ensures that the request has an X-Forwarded-For
109138
// header. It uses the following logic:
110139
//

‎coderd/httpmw/realip_test.go‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/stretchr/testify/require"
1313

14+
"github.com/coder/coder/v2/coderd/httpapi"
1415
"github.com/coder/coder/v2/coderd/httpmw"
1516
)
1617

@@ -472,6 +473,112 @@ func TestFilterUntrusted(t *testing.T) {
472473
}
473474
}
474475

476+
func TestEffectiveHost(t *testing.T) {
477+
t.Parallel()
478+
479+
cidr32 := func(t *testing.T, ip string) *net.IPNet {
480+
t.Helper()
481+
482+
return &net.IPNet{
483+
IP: net.ParseIP(ip),
484+
Mask: net.CIDRMask(32, 32),
485+
}
486+
}
487+
488+
t.Run("UntrustedPeerFallsBackToReceivedHost", func(t *testing.T) {
489+
t.Parallel()
490+
491+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
492+
r.RemoteAddr = "17.18.19.20:1234"
493+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
494+
495+
require.Equal(t, "received.test", httpmw.EffectiveHost(nil, r))
496+
})
497+
498+
t.Run("TrustedPeerUsesOriginalRemoteAddrForTrust", func(t *testing.T) {
499+
t.Parallel()
500+
501+
config := &httpmw.RealIPConfig{
502+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
503+
TrustedHeaders: []string{"X-Real-Ip"},
504+
}
505+
506+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
507+
r.RemoteAddr = "17.18.19.20:1234"
508+
// X-Real-Ip causes ExtractRealIP to rewrite r.RemoteAddr, so
509+
// this test can verify trust still uses OriginalRemoteAddr,
510+
// the actual socket peer.
511+
r.Header.Set("X-Real-Ip", "99.88.77.66")
512+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
513+
514+
middleware := httpmw.ExtractRealIP(config)
515+
next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
516+
require.Equal(t, "99.88.77.66", r.RemoteAddr)
517+
require.Equal(t, "app.test.coder.com", httpmw.EffectiveHost(config, r))
518+
})
519+
520+
middleware(next).ServeHTTP(httptest.NewRecorder(), r)
521+
})
522+
523+
t.Run("UntrustedPeerDoesNotHonorForwardedHost", func(t *testing.T) {
524+
t.Parallel()
525+
526+
config := &httpmw.RealIPConfig{
527+
TrustedOrigins: []*net.IPNet{cidr32(t, "99.88.77.66")},
528+
TrustedHeaders: []string{"X-Real-Ip"},
529+
}
530+
531+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
532+
r.RemoteAddr = "17.18.19.20:1234"
533+
r.Header.Set("X-Real-Ip", "99.88.77.66")
534+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
535+
536+
middleware := httpmw.ExtractRealIP(config)
537+
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
538+
require.Equal(t, "17.18.19.20", r.RemoteAddr)
539+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
540+
})
541+
542+
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
543+
})
544+
545+
t.Run("TrustedPeerWithoutForwardedHostFallsBackToReceivedHost", func(t *testing.T) {
546+
t.Parallel()
547+
548+
config := &httpmw.RealIPConfig{
549+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
550+
TrustedHeaders: []string{"X-Real-Ip"},
551+
}
552+
553+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
554+
r.RemoteAddr = "17.18.19.20:1234"
555+
556+
middleware := httpmw.ExtractRealIP(config)
557+
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
558+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
559+
})
560+
561+
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
562+
})
563+
564+
t.Run("MalformedRemoteAddrFallsBackToReceivedHost", func(t *testing.T) {
565+
t.Parallel()
566+
567+
config := &httpmw.RealIPConfig{
568+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
569+
TrustedHeaders: []string{"X-Real-Ip"},
570+
}
571+
572+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
573+
// A RemoteAddr that cannot be parsed into an IP must be treated as
574+
// untrusted, so the forwarded host is ignored.
575+
r.RemoteAddr = "garbage"
576+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
577+
578+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
579+
})
580+
}
581+
475582
// TestApplicationProxy checks headers passed to DevURL services are as expected.
476583
func TestApplicationProxy(t *testing.T) {
477584
t.Parallel()

‎coderd/workspaceapps/proxy.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,7 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler)
437437
}
438438

439439
// Step 2: Get the request Host.
440-
host := httpapi.RequestHost(r)
440+
host := httpmw.EffectiveHost(s.RealIPConfig, r)
441441
if host == "" {
442442
if r.URL.Path == "/derp" {
443443
// The /derp endpoint is used by wireguard clients to tunnel

0 commit comments

Comments
 (0)