diff --git a/caddytest/integration/caddyfile_adapt/forward_auth_authelia.caddyfiletest b/caddytest/integration/caddyfile_adapt/forward_auth_authelia.caddyfiletest index 240bdc62f..831d7d2fb 100644 --- a/caddytest/integration/caddyfile_adapt/forward_auth_authelia.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/forward_auth_authelia.caddyfiletest @@ -46,6 +46,18 @@ app.example.com { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "Remote-Email" + ] + } + } + ] + }, { "handle": [ { @@ -73,6 +85,18 @@ app.example.com { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "Remote-Groups" + ] + } + } + ] + }, { "handle": [ { @@ -100,6 +124,18 @@ app.example.com { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "Remote-Name" + ] + } + } + ] + }, { "handle": [ { @@ -127,6 +163,18 @@ app.example.com { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "Remote-User" + ] + } + } + ] + }, { "handle": [ { @@ -200,4 +248,4 @@ app.example.com { } } } -} \ No newline at end of file +} diff --git a/caddytest/integration/caddyfile_adapt/forward_auth_copy_headers_strip.caddyfiletest b/caddytest/integration/caddyfile_adapt/forward_auth_copy_headers_strip.caddyfiletest new file mode 100644 index 000000000..887bef0ab --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/forward_auth_copy_headers_strip.caddyfiletest @@ -0,0 +1,146 @@ +:8080 + +forward_auth 127.0.0.1:9091 { + uri / + copy_headers X-User-Id X-User-Role +} +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":8080" + ], + "routes": [ + { + "handle": [ + { + "handle_response": [ + { + "match": { + "status_code": [ + 2 + ] + }, + "routes": [ + { + "handle": [ + { + "handler": "vars" + } + ] + }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "X-User-Id" + ] + } + } + ] + }, + { + "handle": [ + { + "handler": "headers", + "request": { + "set": { + "X-User-Id": [ + "{http.reverse_proxy.header.X-User-Id}" + ] + } + } + } + ], + "match": [ + { + "not": [ + { + "vars": { + "{http.reverse_proxy.header.X-User-Id}": [ + "" + ] + } + } + ] + } + ] + }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "X-User-Role" + ] + } + } + ] + }, + { + "handle": [ + { + "handler": "headers", + "request": { + "set": { + "X-User-Role": [ + "{http.reverse_proxy.header.X-User-Role}" + ] + } + } + } + ], + "match": [ + { + "not": [ + { + "vars": { + "{http.reverse_proxy.header.X-User-Role}": [ + "" + ] + } + } + ] + } + ] + } + ] + } + ], + "handler": "reverse_proxy", + "headers": { + "request": { + "set": { + "X-Forwarded-Method": [ + "{http.request.method}" + ], + "X-Forwarded-Uri": [ + "{http.request.uri}" + ] + } + } + }, + "rewrite": { + "method": "GET", + "uri": "/" + }, + "upstreams": [ + { + "dial": "127.0.0.1:9091" + } + ] + } + ] + } + ] + } + } + } + } +} \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/forward_auth_rename_headers.caddyfiletest b/caddytest/integration/caddyfile_adapt/forward_auth_rename_headers.caddyfiletest index c2be2ed43..5d61e5ff2 100644 --- a/caddytest/integration/caddyfile_adapt/forward_auth_rename_headers.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/forward_auth_rename_headers.caddyfiletest @@ -35,6 +35,18 @@ forward_auth localhost:9000 { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "1" + ] + } + } + ] + }, { "handle": [ { @@ -62,6 +74,18 @@ forward_auth localhost:9000 { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "B" + ] + } + } + ] + }, { "handle": [ { @@ -89,6 +113,18 @@ forward_auth localhost:9000 { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "3" + ] + } + } + ] + }, { "handle": [ { @@ -116,6 +152,18 @@ forward_auth localhost:9000 { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "D" + ] + } + } + ] + }, { "handle": [ { @@ -143,6 +191,18 @@ forward_auth localhost:9000 { } ] }, + { + "handle": [ + { + "handler": "headers", + "request": { + "delete": [ + "5" + ] + } + } + ] + }, { "handle": [ { @@ -203,4 +263,4 @@ forward_auth localhost:9000 { } } } -} \ No newline at end of file +} diff --git a/caddytest/integration/forwardauth_test.go b/caddytest/integration/forwardauth_test.go new file mode 100644 index 000000000..d0ecc2be1 --- /dev/null +++ b/caddytest/integration/forwardauth_test.go @@ -0,0 +1,206 @@ +// Copyright 2015 Matthew Holt and The Caddy Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package integration + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/caddyserver/caddy/v2/caddytest" +) + +// TestForwardAuthCopyHeadersStripsClientHeaders is a regression test for the +// header injection vulnerability in forward_auth copy_headers. +// +// When the auth service returns 200 OK without one of the copy_headers headers, +// the MatchNot guard skips the Set operation. Before this fix, the original +// client-supplied header survived unchanged into the backend request, allowing +// privilege escalation with only a valid (non-privileged) bearer token. After +// the fix, an unconditional delete route runs first, so the backend always +// sees an absent header rather than the attacker-supplied value. +func TestForwardAuthCopyHeadersStripsClientHeaders(t *testing.T) { + // Mock auth service: accepts any Bearer token, returns 200 OK with NO + // identity headers. This is the stateless JWT validator pattern that + // triggers the vulnerability. + authSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.Header.Get("Authorization"), "Bearer ") { + w.WriteHeader(http.StatusOK) + return + } + w.WriteHeader(http.StatusUnauthorized) + })) + defer authSrv.Close() + + // Mock backend: records the identity headers it receives. A real application + // would use X-User-Id / X-User-Role to make authorization decisions. + type received struct{ userID, userRole string } + var ( + mu sync.Mutex + last received + ) + backendSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + last = received{ + userID: r.Header.Get("X-User-Id"), + userRole: r.Header.Get("X-User-Role"), + } + mu.Unlock() + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "ok") + })) + defer backendSrv.Close() + + authAddr := strings.TrimPrefix(authSrv.URL, "http://") + backendAddr := strings.TrimPrefix(backendSrv.URL, "http://") + + tester := caddytest.NewTester(t) + tester.InitServer(fmt.Sprintf(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + http://localhost:9080 { + forward_auth %s { + uri / + copy_headers X-User-Id X-User-Role + } + reverse_proxy %s + } + `, authAddr, backendAddr), "caddyfile") + + // Case 1: no token. Auth must still reject the request even when the client + // includes identity headers. This confirms the auth check is not bypassed. + req, _ := http.NewRequest(http.MethodGet, "http://localhost:9080/", nil) + req.Header.Set("X-User-Id", "injected") + req.Header.Set("X-User-Role", "injected") + resp := tester.AssertResponseCode(req, http.StatusUnauthorized) + resp.Body.Close() + + // Case 2: valid token, no injected headers. The backend should see absent + // identity headers (the auth service never returns them). + req, _ = http.NewRequest(http.MethodGet, "http://localhost:9080/", nil) + req.Header.Set("Authorization", "Bearer token123") + tester.AssertResponse(req, http.StatusOK, "ok") + mu.Lock() + gotID, gotRole := last.userID, last.userRole + mu.Unlock() + if gotID != "" { + t.Errorf("baseline: X-User-Id should be absent, got %q", gotID) + } + if gotRole != "" { + t.Errorf("baseline: X-User-Role should be absent, got %q", gotRole) + } + + // Case 3 (the security regression): valid token plus forged identity headers. + // The fix must strip those values so the backend never sees them. + req, _ = http.NewRequest(http.MethodGet, "http://localhost:9080/", nil) + req.Header.Set("Authorization", "Bearer token123") + req.Header.Set("X-User-Id", "admin") // forged + req.Header.Set("X-User-Role", "superadmin") // forged + tester.AssertResponse(req, http.StatusOK, "ok") + mu.Lock() + gotID, gotRole = last.userID, last.userRole + mu.Unlock() + if gotID != "" { + t.Errorf("injection: X-User-Id must be stripped, got %q", gotID) + } + if gotRole != "" { + t.Errorf("injection: X-User-Role must be stripped, got %q", gotRole) + } +} + +// TestForwardAuthCopyHeadersAuthResponseWins verifies that when the auth +// service does include a copy_headers header in its response, that value +// is forwarded to the backend and takes precedence over any client-supplied +// value for the same header. +func TestForwardAuthCopyHeadersAuthResponseWins(t *testing.T) { + const wantUserID = "service-user-42" + const wantUserRole = "editor" + + // Auth service: accepts bearer token and sets identity headers. + authSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.Header.Get("Authorization"), "Bearer ") { + w.Header().Set("X-User-Id", wantUserID) + w.Header().Set("X-User-Role", wantUserRole) + w.WriteHeader(http.StatusOK) + return + } + w.WriteHeader(http.StatusUnauthorized) + })) + defer authSrv.Close() + + type received struct{ userID, userRole string } + var ( + mu sync.Mutex + last received + ) + backendSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + last = received{ + userID: r.Header.Get("X-User-Id"), + userRole: r.Header.Get("X-User-Role"), + } + mu.Unlock() + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "ok") + })) + defer backendSrv.Close() + + authAddr := strings.TrimPrefix(authSrv.URL, "http://") + backendAddr := strings.TrimPrefix(backendSrv.URL, "http://") + + tester := caddytest.NewTester(t) + tester.InitServer(fmt.Sprintf(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + http://localhost:9080 { + forward_auth %s { + uri / + copy_headers X-User-Id X-User-Role + } + reverse_proxy %s + } + `, authAddr, backendAddr), "caddyfile") + + // The client sends forged headers; the auth service overrides them with + // its own values. The backend must receive the auth service values. + req, _ := http.NewRequest(http.MethodGet, "http://localhost:9080/", nil) + req.Header.Set("Authorization", "Bearer token123") + req.Header.Set("X-User-Id", "forged-id") // must be overwritten + req.Header.Set("X-User-Role", "forged-role") // must be overwritten + tester.AssertResponse(req, http.StatusOK, "ok") + + mu.Lock() + gotID, gotRole := last.userID, last.userRole + mu.Unlock() + if gotID != wantUserID { + t.Errorf("X-User-Id: want %q, got %q", wantUserID, gotID) + } + if gotRole != wantUserRole { + t.Errorf("X-User-Role: want %q, got %q", wantUserRole, gotRole) + } +} diff --git a/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go b/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go index f838c8702..1273e906c 100644 --- a/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go +++ b/modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go @@ -208,6 +208,24 @@ func parseCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) for _, from := range sortedHeadersToCopy { to := http.CanonicalHeaderKey(headersToCopy[from]) placeholderName := "http.reverse_proxy.header." + http.CanonicalHeaderKey(from) + + // Always delete the client-supplied header before conditionally setting + // it from the auth response. Without this, a client that pre-supplies a + // header listed in copy_headers can inject arbitrary values when the auth + // service does not return that header: the MatchNot guard below would + // skip the Set entirely, leaving the original client-controlled value + // intact and forwarding it to the backend. + copyHeaderRoutes = append(copyHeaderRoutes, caddyhttp.Route{ + HandlersRaw: []json.RawMessage{caddyconfig.JSONModuleObject( + &headers.Handler{ + Request: &headers.HeaderOps{ + Delete: []string{to}, + }, + }, + "handler", "headers", nil, + )}, + }) + handler := &headers.Handler{ Request: &headers.HeaderOps{ Set: http.Header{