From 42be134173abd99527ae19bcbe4d72827b8d2403 Mon Sep 17 00:00:00 2001 From: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com> Date: Wed, 30 Aug 2023 14:19:21 -0700 Subject: [PATCH] authorize: compute "real" IP from X-Forwarded-For WIP -- this doesn't work correctly yet Update the authorize log IP address field to match the access log IP address field. Add a new method getIPAddress() to compute the "real" client IP address based on the contents of the X-Forwarded-For header and the XffNumTrustedHops and SkipXffAppend configuration option. This is intended to match downstream_remote_address from the access log: https://www.envoyproxy.io/docs/envoy/latest/api-v3/data/accesslog/v3/accesslog.proto#envoy-v3-api-field-data-accesslog-v3-accesslogcommon-downstream-remote-address --- authorize/log.go | 40 ++++++++++++++++++++-- authorize/log_test.go | 79 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/authorize/log.go b/authorize/log.go index a530e8a6a..677f6fa2a 100644 --- a/authorize/log.go +++ b/authorize/log.go @@ -9,6 +9,7 @@ import ( "github.com/rs/zerolog" "github.com/pomerium/pomerium/authorize/evaluator" + "github.com/pomerium/pomerium/config" "github.com/pomerium/pomerium/internal/log" "github.com/pomerium/pomerium/internal/telemetry/requestid" "github.com/pomerium/pomerium/internal/telemetry/trace" @@ -32,9 +33,10 @@ func (a *Authorize) logAuthorizeCheck( impersonateDetails := a.getImpersonateDetails(ctx, s) evt := log.Info(ctx).Str("service", "authorize") - fields := a.currentOptions.Load().GetAuthorizeLogFields() + currentOptions := a.currentOptions.Load() + fields := currentOptions.GetAuthorizeLogFields() for _, field := range fields { - evt = populateLogEvent(ctx, field, evt, in, s, u, hdrs, impersonateDetails) + evt = populateLogEvent(ctx, field, evt, in, s, u, hdrs, impersonateDetails, currentOptions) } evt = log.HTTPHeaders(evt, fields, hdrs) @@ -152,6 +154,7 @@ func populateLogEvent( u *user.User, hdrs map[string]string, impersonateDetails *impersonateDetails, + currentOptions *config.Options, ) *zerolog.Event { path, query, _ := strings.Cut(in.GetAttributes().GetRequest().GetHttp().GetPath(), "?") @@ -192,7 +195,7 @@ func populateLogEvent( } return evt case log.AuthorizeLogFieldIP: - return evt.Str(string(field), in.GetAttributes().GetSource().GetAddress().GetSocketAddress().GetAddress()) + return evt.Str(string(field), getIPAddress(in.GetAttributes(), currentOptions)) case log.AuthorizeLogFieldMethod: return evt.Str(string(field), in.GetAttributes().GetRequest().GetHttp().GetMethod()) case log.AuthorizeLogFieldPath: @@ -217,3 +220,34 @@ func populateLogEvent( return evt } } + +func getIPAddress(attributes *envoy_service_auth_v3.AttributeContext, cfg *config.Options) string { + socketAddress := attributes.GetSource().GetAddress().GetSocketAddress().GetAddress() + xForwardedFor := attributes.GetRequest().GetHttp().GetHeaders()["x-forwarded-for"] + + // If XffNumTrustedHops is zero, we should not trust anything in the + // X-Forwarded-For header, so return the actual socket address. + // If the X-Forwarded-For header is empty, we should not trust it either. + if cfg.XffNumTrustedHops == 0 || xForwardedFor == "" { + return socketAddress + } + + // Parse the X-Forwarded-For header by splitting on the ',' character. + addresses := strings.Split(xForwardedFor, ",") + + // Unless SkipXffAppend is set, the last address in X-Forwarded-For was + // appended by Envoy, so it shouldn't count as a separate hop. + hops := cfg.XffNumTrustedHops + if !cfg.SkipXffAppend { + hops++ + } + + // Fall back to the actual socket address if the X-Forwarded-For header has + // fewer entries than expected (or if XffNumTrustedHops was set to exactly + // the maximum uint32 value and SkipXffAppend is false). + lenAddresses := uint32(len(addresses)) + if hops > lenAddresses || hops == 0 { + return socketAddress + } + return strings.TrimSpace(addresses[lenAddresses-hops]) +} diff --git a/authorize/log_test.go b/authorize/log_test.go index b20c3c573..aa758f89c 100644 --- a/authorize/log_test.go +++ b/authorize/log_test.go @@ -11,6 +11,7 @@ import ( "github.com/rs/zerolog" "github.com/stretchr/testify/assert" + "github.com/pomerium/pomerium/config" "github.com/pomerium/pomerium/internal/log" "github.com/pomerium/pomerium/internal/telemetry/requestid" "github.com/pomerium/pomerium/pkg/grpc/session" @@ -62,6 +63,7 @@ func Test_populateLogEvent(t *testing.T) { sessionID: "IMPERSONATE-SESSION-ID", userID: "IMPERSONATE-USER-ID", } + opts := &config.Options{} for _, tc := range []struct { field log.AuthorizeLogField @@ -93,10 +95,85 @@ func Test_populateLogEvent(t *testing.T) { var buf bytes.Buffer log := zerolog.New(&buf) evt := log.Log() - evt = populateLogEvent(ctx, tc.field, evt, checkRequest, tc.s, u, headers, impersonateDetails) + evt = populateLogEvent(ctx, tc.field, evt, checkRequest, tc.s, u, headers, impersonateDetails, opts) evt.Send() assert.Equal(t, tc.expect, strings.TrimSpace(buf.String())) }) } } + +func Test_getIPAddress(t *testing.T) { + t.Parallel() + + cases := []struct { + label string + socketIP string + xForwardedFor string + xffNumTrustedHops uint32 + skipXffAppend bool + expected string + }{ + {"xff_num_trusted_hops=0", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 0, false, "127.0.0.1"}, + {"xff_num_trusted_hops=1", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 1, false, "10.1.1.1"}, + {"xff_num_trusted_hops=2", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 2, false, "10.2.2.2"}, + {"xff_num_trusted_hops=3", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 3, false, "127.0.0.1"}, + {"xff_num_trusted_hops=4", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 4, false, "127.0.0.1"}, + + {"xff_num_trusted_hops=0 skip_xff_append", + "127.0.0.1", "10.2.2.2, 10.1.1.1", 0, true, "127.0.0.1"}, + {"xff_num_trusted_hops=1 skip_xff_append", + "127.0.0.1", "10.2.2.2, 10.1.1.1", 1, true, "10.1.1.1"}, + {"xff_num_trusted_hops=2 skip_xff_append", + "127.0.0.1", "10.2.2.2, 10.1.1.1", 2, true, "10.2.2.2"}, + {"xff_num_trusted_hops=3 skip_xff_append", + "127.0.0.1", "10.2.2.2, 10.1.1.1", 3, true, "127.0.0.1"}, + {"xff_num_trusted_hops=4 skip_xff_append", + "127.0.0.1", "10.2.2.2, 10.1.1.1", 4, true, "127.0.0.1"}, + + {"xff_num_trusted_hops int32 overflow", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 0x80000000, false, "127.0.0.1"}, + {"xff_num_trusted_hops uint32 max", + "127.0.0.1", "10.2.2.2, 10.1.1.1, 10.0.0.0", 0xffffffff, false, "127.0.0.1"}, + + {"empty X-Forwarded-For", + "127.0.0.1", "", 1, false, "127.0.0.1"}, + {"empty X-Forwarded-For skip_xff_append", + "127.0.0.1", "", 1, true, "127.0.0.1"}, + } + + for i := range cases { + c := &cases[i] + t.Run(c.label, func(t *testing.T) { + attributes := &envoy_service_auth_v3.AttributeContext{ + Request: &envoy_service_auth_v3.AttributeContext_Request{ + Http: &envoy_service_auth_v3.AttributeContext_HttpRequest{ + Headers: map[string]string{ + "x-forwarded-for": c.xForwardedFor, + }, + }, + }, + Source: &envoy_service_auth_v3.AttributeContext_Peer{ + Address: &envoy_config_core_v3.Address{ + Address: &envoy_config_core_v3.Address_SocketAddress{ + SocketAddress: &envoy_config_core_v3.SocketAddress{ + Address: c.socketIP, + }, + }, + }, + }, + } + options := &config.Options{ + XffNumTrustedHops: c.xffNumTrustedHops, + SkipXffAppend: c.skipXffAppend, + } + actual := getIPAddress(attributes, options) + assert.Equal(t, c.expected, actual) + }) + } +}