From 33d4e4843b866979dceebd857db48b2762cf3707 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Sat, 28 Sep 2019 12:15:13 -0700 Subject: [PATCH] internal/log: return full `X-Forwarded-For` Signed-off-by: Bobby DeSimone --- docs/docs/CHANGELOG.md | 1 + internal/log/middleware.go | 17 +++++++---------- internal/log/middleware_test.go | 12 ++++++------ 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/docs/docs/CHANGELOG.md b/docs/docs/CHANGELOG.md index 3610c8ca0..baec9fcb3 100644 --- a/docs/docs/CHANGELOG.md +++ b/docs/docs/CHANGELOG.md @@ -22,6 +22,7 @@ - The healthcheck endpoints (`/ping`) now returns the http status `405` StatusMethodNotAllowed for non-`GET` requests. [GH-319](https://github.com/pomerium/pomerium/issues/319) - Authenticate service no longer uses gRPC. +- The global request logger now captures the full array of proxies from `X-Forwarded-For`, in addition to just the client IP. ### Removed diff --git a/internal/log/middleware.go b/internal/log/middleware.go index 4925e8802..2cc6867bb 100644 --- a/internal/log/middleware.go +++ b/internal/log/middleware.go @@ -172,22 +172,19 @@ func AccessHandler(f func(r *http.Request, status, size int, duration time.Durat } } -// ForwardedAddrHandler returns the client IP address from a request. If present, the -// X-Forwarded-For header is assumed to be set by a load balancer, and its -// rightmost entry (the client IP that connected to the LB) is returned. +// ForwardedAddrHandler returns the client IP address from a request. If a +// request goes through multiple proxies, the IP addresses of each successive +// proxy is listed. This means, the right-most IP address is the IP address of +// the most recent proxy and the left-most IP address is the IP address of the +// originating client. +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For func ForwardedAddrHandler(fieldKey string) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - addr := r.RemoteAddr if ra := r.Header.Get("X-Forwarded-For"); ra != "" { - forwardedList := strings.Split(ra, ",") - forwardedAddr := strings.TrimSpace(forwardedList[len(forwardedList)-1]) - if forwardedAddr != "" { - addr = forwardedAddr - } log := zerolog.Ctx(r.Context()) log.UpdateContext(func(c zerolog.Context) zerolog.Context { - return c.Str(fieldKey, addr) + return c.Strs(fieldKey, strings.Split(ra, ",")) }) } next.ServeHTTP(w, r) diff --git a/internal/log/middleware_test.go b/internal/log/middleware_test.go index a3e391bb1..067dac6b8 100644 --- a/internal/log/middleware_test.go +++ b/internal/log/middleware_test.go @@ -253,18 +253,18 @@ func BenchmarkDataRace(b *testing.B) { func TestForwardedAddrHandler(t *testing.T) { out := &bytes.Buffer{} - r := &http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"client", "proxy1", "proxy2"}, - }, - } + + r := httptest.NewRequest(http.MethodGet, "/", nil) + + r.Header.Set("X-Forwarded-For", "proxy1,proxy2,proxy3") + h := ForwardedAddrHandler("fwd_ip")(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { l := FromRequest(r) l.Log().Msg("") })) h = NewHandler(zerolog.New(out))(h) h.ServeHTTP(nil, r) - if want, got := `{"fwd_ip":"client"}`+"\n", decodeIfBinary(out); want != got { + if want, got := `{"fwd_ip":["proxy1","proxy2","proxy3"]}`+"\n", decodeIfBinary(out); want != got { t.Errorf("Invalid log output, got: %s, want: %s", got, want) } }