From 436e65300792a5be83dce27b24312d43ab1d1649 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Tue, 11 Aug 2020 10:49:01 +0100 Subject: [PATCH 01/14] Log X-Forwarded-For for every request Signed-off-by: Michel Hollands --- go.mod | 10 ---- go.sum | 1 + middleware/forwarded.go | 42 +++++++++++++++ middleware/forwarded_test.go | 100 +++++++++++++++++++++++++++++++++++ middleware/logging.go | 6 +++ server/server.go | 42 +++++++++------ server/server_test.go | 36 +++++++++++-- 7 files changed, 207 insertions(+), 30 deletions(-) create mode 100644 middleware/forwarded.go create mode 100644 middleware/forwarded_test.go diff --git a/go.mod b/go.mod index d72adcda..31f45e35 100644 --- a/go.mod +++ b/go.mod @@ -12,24 +12,15 @@ require ( github.com/gogo/status v1.0.3 github.com/golang/lint v0.0.0-20180702182130-06c8688daad7 // indirect github.com/golang/protobuf v1.3.3 - github.com/gorilla/context v1.1.1 // indirect github.com/gorilla/mux v1.7.3 github.com/grpc-ecosystem/go-grpc-middleware v1.1.0 - github.com/konsorten/go-windows-terminal-sequences v1.0.1 // indirect - github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515 // indirect - github.com/mattn/go-colorable v0.0.9 // indirect - github.com/mattn/go-isatty v0.0.4 // indirect - github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/opentracing-contrib/go-grpc v0.0.0-20180928155321-4b5a12d3ff02 github.com/opentracing-contrib/go-stdlib v0.0.0-20190519235532-cf7a6c988dc9 github.com/opentracing/opentracing-go v1.1.0 github.com/pkg/errors v0.9.1 github.com/pmezard/go-difflib v1.0.0 github.com/prometheus/client_golang v1.4.1 - github.com/prometheus/common v0.9.1 // indirect github.com/prometheus/node_exporter v1.0.0-rc.0.0.20200428091818-01054558c289 github.com/sercand/kuberesolver v2.1.0+incompatible github.com/sirupsen/logrus v1.4.2 @@ -39,7 +30,6 @@ require ( github.com/weaveworks/promrus v1.2.0 golang.org/x/net v0.0.0-20200202094626-16171245cfb2 golang.org/x/tools v0.0.0-20200216192241-b320d3a0f5a2 - google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55 // indirect google.golang.org/grpc v1.26.0 gopkg.in/yaml.v2 v2.2.8 ) diff --git a/go.sum b/go.sum index de35f58d..80dbc10b 100644 --- a/go.sum +++ b/go.sum @@ -279,6 +279,7 @@ github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5Fsn github.com/prometheus/client_golang v1.3.0/go.mod h1:hJaj2vgQTGQmVCsAACORcieXFeDPbaTKGT+JTgUa3og= github.com/prometheus/client_golang v1.4.1 h1:FFSuS004yOQEtDdTq+TAOLP5xUq63KqAFYyOi8zA+Y8= github.com/prometheus/client_golang v1.4.1/go.mod h1:e9GMxYsXl05ICDXkRhurwBS4Q3OK1iX/F2sw+iXX5zU= +github.com/prometheus/client_golang v1.7.1 h1:NTGy1Ja9pByO+xAeH/qiWnLrKtr3hJPNjaVUwnjpdpA= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190115171406-56726106282f/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 h1:S/YWwWx/RA8rT8tKFRuGUZhuA90OyIBpPCXkcbwU8DE= diff --git a/middleware/forwarded.go b/middleware/forwarded.go new file mode 100644 index 00000000..b3ff2981 --- /dev/null +++ b/middleware/forwarded.go @@ -0,0 +1,42 @@ +package middleware + +import ( + "fmt" + "net" + "net/http" +) + +// extractHost returns the Host IP address without any port information +func extractHost(address string) string { + hostIP := net.ParseIP(address) + if hostIP != nil { + return hostIP.String() + } + var err error + hostStr, _, err := net.SplitHostPort(address) + if err != nil { + // Invalid IP address, just return it so it shows up in the logs + return address + } + return hostStr +} + +// GetSource extracts the X-FORWARDED-FOR header from the given HTTP request +// and returns a string with it and the remote address +func GetSource(req *http.Request) string { + fwd := req.Header.Get("X-FORWARDED-FOR") + if fwd == "" { + if req.RemoteAddr == "" { + // No X-FORWARDED-FOR header and no RemoteAddr set so we want to return an empty string + // Might as well just use the empty string set in req.RemoteAddr + return req.RemoteAddr + } + return extractHost(req.RemoteAddr) + } + // If RemoteAddr is empty just return the header + if req.RemoteAddr == "" { + return fwd + } + // If both a header and RemoteAddr are present return them both, stripping off any port info from the RemoteAddr + return fmt.Sprintf("%v, %v", fwd, extractHost(req.RemoteAddr)) +} diff --git a/middleware/forwarded_test.go b/middleware/forwarded_test.go new file mode 100644 index 00000000..2a8dde85 --- /dev/null +++ b/middleware/forwarded_test.go @@ -0,0 +1,100 @@ +package middleware + +import ( + "net/http" + "testing" +) + +func TestGetSource(t *testing.T) { + type args struct { + req *http.Request + } + tests := []struct { + name string + args args + want string + }{ + { + name: "no X-FORWARDED_FOR header", + args: args{ + req: &http.Request{RemoteAddr: "192.168.1.100:3454"}, + }, + want: "192.168.1.100", + }, + { + name: "no X-FORWARDED-FOR header, remote has no port", + args: args{ + req: &http.Request{RemoteAddr: "192.168.1.100"}, + }, + want: "192.168.1.100", + }, + { + name: "no X-FORWARDED-FOR header, remote address is invalid", + args: args{ + req: &http.Request{RemoteAddr: "192.168.100"}, + }, + want: "192.168.100", + }, + { + name: "single forward address", + args: args{ + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1"}, + }, + }, + }, + want: "172.16.1.1, 192.168.1.100", + }, + { + name: "single IPv6 forward address", + args: args{ + req: &http.Request{ + RemoteAddr: "[2001:db9::1]:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"2001:db8::1"}, + }, + }, + }, + want: "2001:db8::1, 2001:db9::1", + }, + { + name: "single forward address no RemoteAddr", + args: args{ + req: &http.Request{ + Header: map[string][]string{ + http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1"}, + }, + }, + }, + want: "172.16.1.1", + }, + { + name: "multiple forward with remote", + args: args{ + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1, 10.10.13.20"}, + }, + }, + }, + want: "172.16.1.1, 10.10.13.20, 192.168.1.100", + }, + { + name: "no forward header, no remote", + args: args{ + req: &http.Request{}, + }, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := GetSource(tt.args.req); got != tt.want { + t.Errorf("GetSource() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/middleware/logging.go b/middleware/logging.go index 79368f9d..c494a49e 100644 --- a/middleware/logging.go +++ b/middleware/logging.go @@ -24,6 +24,12 @@ func (l Log) logWithRequest(r *http.Request) logging.Interface { l.Log = l.Log.WithField("traceID", traceID) } + // Add the source IPs derived from X-Forwarded-For and the request remote address + sourceIPs := GetSource(r) + if sourceIPs != "" { + l.Log = l.Log.WithField("sourceIPs", sourceIPs) + } + return user.LogWith(r.Context(), l.Log) } diff --git a/server/server.go b/server/server.go index 4dc8ae0e..2516aadf 100644 --- a/server/server.go +++ b/server/server.go @@ -62,10 +62,12 @@ type Config struct { HTTPServerWriteTimeout time.Duration `yaml:"http_server_write_timeout"` HTTPServerIdleTimeout time.Duration `yaml:"http_server_idle_timeout"` - GRPCOptions []grpc.ServerOption `yaml:"-"` - GRPCMiddleware []grpc.UnaryServerInterceptor `yaml:"-"` - GRPCStreamMiddleware []grpc.StreamServerInterceptor `yaml:"-"` - HTTPMiddleware []middleware.Interface `yaml:"-"` + GRPCOptions []grpc.ServerOption `yaml:"-"` + GRPCMiddleware []grpc.UnaryServerInterceptor `yaml:"-"` + GRPCStreamMiddleware []grpc.StreamServerInterceptor `yaml:"-"` + HTTPMiddleware []middleware.Interface `yaml:"-"` + Router *mux.Router `yaml:"-"` + DisableGeneratedHTTPMiddleware bool `yaml:"-"` GPRCServerMaxRecvMsgSize int `yaml:"grpc_server_max_recv_msg_size"` GRPCServerMaxSendMsgSize int `yaml:"grpc_server_max_send_msg_size"` @@ -240,7 +242,12 @@ func New(cfg Config) (*Server, error) { grpcServer := grpc.NewServer(grpcOptions...) // Setup HTTP server - router := mux.NewRouter() + var router *mux.Router + if cfg.Router != nil { + router = cfg.Router + } else { + router = mux.NewRouter() + } if cfg.PathPrefix != "" { // Expect metrics and pprof handlers to be prefixed with server's path prefix. // e.g. /loki/metrics or /loki/debug/pprof @@ -249,17 +256,20 @@ func New(cfg Config) (*Server, error) { if cfg.RegisterInstrumentation { RegisterInstrumentation(router) } - httpMiddleware := []middleware.Interface{ - middleware.Tracer{ - RouteMatcher: router, - }, - middleware.Log{ - Log: log, - }, - middleware.Instrument{ - Duration: requestDuration, - RouteMatcher: router, - }, + httpMiddleware := []middleware.Interface{} + if !cfg.DisableGeneratedHTTPMiddleware { + httpMiddleware = append(httpMiddleware, + middleware.Tracer{ + RouteMatcher: router, + }, + middleware.Log{ + Log: log, + }, + middleware.Instrument{ + Duration: requestDuration, + RouteMatcher: router, + }, + ) } httpMiddleware = append(httpMiddleware, cfg.HTTPMiddleware...) diff --git a/server/server_test.go b/server/server_test.go index f48c0dd0..58a4bd73 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -17,6 +17,7 @@ import ( "google.golang.org/grpc/status" google_protobuf "github.com/golang/protobuf/ptypes/empty" + "github.com/gorilla/mux" "github.com/prometheus/client_golang/prometheus" node_https "github.com/prometheus/node_exporter/https" "github.com/stretchr/testify/require" @@ -294,6 +295,33 @@ func TestMiddlewareLogging(t *testing.T) { http.DefaultClient.Do(req) } +func TestMuxMiddleware(t *testing.T) { + var level logging.Level + level.Set("info") + cfg := Config{ + HTTPListenAddress: "localhost", + HTTPListenPort: 9193, + GRPCListenAddress: "localhost", + HTTPMiddleware: []middleware.Interface{middleware.Logging}, + MetricsNamespace: "testing_mux", + LogLevel: level, + Router: mux.NewRouter(), + } + server, err := New(cfg) + require.NoError(t, err) + + server.HTTP.HandleFunc("/error500", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + }) + + go server.Run() + defer server.Shutdown() + + req, err := http.NewRequest("GET", "http://127.0.0.1:9192/error500", nil) + require.NoError(t, err) + http.DefaultClient.Do(req) +} + func TestTLSServer(t *testing.T) { var level logging.Level level.Set("info") @@ -304,7 +332,7 @@ func TestTLSServer(t *testing.T) { cfg := Config{ HTTPListenAddress: "localhost", - HTTPListenPort: 9193, + HTTPListenPort: 9194, HTTPTLSConfig: node_https.TLSStruct{ TLSCertPath: "certs/server.crt", TLSKeyPath: "certs/server.key", @@ -319,7 +347,7 @@ func TestTLSServer(t *testing.T) { }, MetricsNamespace: "testing_tls", GRPCListenAddress: "localhost", - GRPCListenPort: 9194, + GRPCListenPort: 9195, } server, err := New(cfg) require.NoError(t, err) @@ -353,7 +381,7 @@ func TestTLSServer(t *testing.T) { } client := &http.Client{Transport: tr} - res, err := client.Get("https://localhost:9193/testhttps") + res, err := client.Get("https://localhost:9194/testhttps") require.NoError(t, err) defer res.Body.Close() @@ -364,7 +392,7 @@ func TestTLSServer(t *testing.T) { expected := []byte("Hello World!") require.Equal(t, expected, body) - conn, err := grpc.Dial("localhost:9194", grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) + conn, err := grpc.Dial("localhost:9195", grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) defer conn.Close() require.NoError(t, err) From bc9a4d1a336b54876c53803eaaf6ce2e8fac00e2 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Wed, 12 Aug 2020 15:53:43 +0100 Subject: [PATCH 02/14] Address review comments and add Forwarded and X-Real-IP support Signed-off-by: Michel Hollands --- middleware/forwarded.go | 75 +++++++++++++-- middleware/forwarded_test.go | 180 +++++++++++++++++++++++++---------- middleware/logging.go | 7 +- server/server_test.go | 12 +-- 4 files changed, 210 insertions(+), 64 deletions(-) diff --git a/middleware/forwarded.go b/middleware/forwarded.go index b3ff2981..2d89d6fd 100644 --- a/middleware/forwarded.go +++ b/middleware/forwarded.go @@ -4,6 +4,32 @@ import ( "fmt" "net" "net/http" + "regexp" + "strings" +) + +// Parts copied and changed from gorilla mux proxy_headers.go + +var ( + // De-facto standard header keys. + xForwardedFor = http.CanonicalHeaderKey("X-Forwarded-For") + xForwardedHost = http.CanonicalHeaderKey("X-Forwarded-Host") + xForwardedProto = http.CanonicalHeaderKey("X-Forwarded-Proto") + xForwardedScheme = http.CanonicalHeaderKey("X-Forwarded-Scheme") + xRealIP = http.CanonicalHeaderKey("X-Real-IP") +) + +var ( + // RFC7239 defines a new "Forwarded: " header designed to replace the + // existing use of X-Forwarded-* headers. + // e.g. Forwarded: for=192.0.2.60;proto=https;by=203.0.113.43 + forwarded = http.CanonicalHeaderKey("Forwarded") + // Allows for a sub-match of the first value after 'for=' to the next + // comma, semi-colon or space. The match is case-insensitive. + forRegex = regexp.MustCompile(`(?i)(?:for=)([^(;|,| )]+)`) + // Allows for a sub-match for the first instance of scheme (http|https) + // prefixed by 'proto='. The match is case-insensitive. + protoRegex = regexp.MustCompile(`(?i)(?:proto=)(https|http)`) ) // extractHost returns the Host IP address without any port information @@ -21,15 +47,12 @@ func extractHost(address string) string { return hostStr } -// GetSource extracts the X-FORWARDED-FOR header from the given HTTP request -// and returns a string with it and the remote address +// GetSource returns any source addresses we can find in the request, comma-separated func GetSource(req *http.Request) string { - fwd := req.Header.Get("X-FORWARDED-FOR") + fwd := extractHost(getIP(req)) if fwd == "" { if req.RemoteAddr == "" { - // No X-FORWARDED-FOR header and no RemoteAddr set so we want to return an empty string - // Might as well just use the empty string set in req.RemoteAddr - return req.RemoteAddr + return "" } return extractHost(req.RemoteAddr) } @@ -37,6 +60,44 @@ func GetSource(req *http.Request) string { if req.RemoteAddr == "" { return fwd } + remoteIP := extractHost(req.RemoteAddr) + if fwd == remoteIP { + return remoteIP + } // If both a header and RemoteAddr are present return them both, stripping off any port info from the RemoteAddr - return fmt.Sprintf("%v, %v", fwd, extractHost(req.RemoteAddr)) + return fmt.Sprintf("%v, %v", fwd, remoteIP) +} + +// getIP retrieves the IP from the RFC7239 Forwarded headers, +// X-Real-IP and X-Forwarded-For (in that order). +func getIP(r *http.Request) string { + var addr string + + if fwd := r.Header.Get(forwarded); fwd != "" { + // match should contain at least two elements if the protocol was + // specified in the Forwarded header. The first element will always be + // the 'for=' capture, which we ignore. In the case of multiple IP + // addresses (for=8.8.8.8, 8.8.4.4,172.16.1.20 is valid) we only + // extract the first, which should be the client IP. + if match := forRegex.FindStringSubmatch(fwd); len(match) > 1 { + // IPv6 addresses in Forwarded headers are quoted-strings. We strip + // these quotes. + addr = strings.Trim(match[1], `"`) + } + } else if fwd := r.Header.Get(xRealIP); fwd != "" { + // X-Real-IP should only contain one IP address (the client making the + // request). + addr = fwd + } else if fwd := r.Header.Get(xForwardedFor); fwd != "" { + // Only grab the first (client) address. Note that '192.168.0.1, + // 10.1.1.1' is a valid key for X-Forwarded-For where addresses after + // the first may represent forwarding proxies earlier in the chain. + s := strings.Index(fwd, ", ") + if s == -1 { + s = len(fwd) + } + addr = fwd[:s] + } + + return addr } diff --git a/middleware/forwarded_test.go b/middleware/forwarded_test.go index 2a8dde85..7586c4fc 100644 --- a/middleware/forwarded_test.go +++ b/middleware/forwarded_test.go @@ -6,93 +6,177 @@ import ( ) func TestGetSource(t *testing.T) { - type args struct { - req *http.Request - } tests := []struct { name string - args args + req *http.Request want string }{ { - name: "no X-FORWARDED_FOR header", - args: args{ - req: &http.Request{RemoteAddr: "192.168.1.100:3454"}, - }, + name: "no header", + req: &http.Request{RemoteAddr: "192.168.1.100:3454"}, want: "192.168.1.100", }, { - name: "no X-FORWARDED-FOR header, remote has no port", - args: args{ - req: &http.Request{RemoteAddr: "192.168.1.100"}, - }, + name: "no header and remote has no port", + req: &http.Request{RemoteAddr: "192.168.1.100"}, want: "192.168.1.100", }, { - name: "no X-FORWARDED-FOR header, remote address is invalid", - args: args{ - req: &http.Request{RemoteAddr: "192.168.100"}, - }, + name: "no header, remote address is invalid", + req: &http.Request{RemoteAddr: "192.168.100"}, want: "192.168.100", }, { - name: "single forward address", - args: args{ - req: &http.Request{ - RemoteAddr: "192.168.1.100:3454", - Header: map[string][]string{ - http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1"}, - }, + name: "X-Forwarded-For and single forward address", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"172.16.1.1"}, }, }, want: "172.16.1.1, 192.168.1.100", }, { - name: "single IPv6 forward address", - args: args{ - req: &http.Request{ - RemoteAddr: "[2001:db9::1]:3454", - Header: map[string][]string{ - http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"2001:db8::1"}, - }, + name: "X-Forwarded-For and single forward address which is same as remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"192.168.1.100"}, + }, + }, + want: "192.168.1.100", + }, + { + name: "single IPv6 X-Forwarded-For address", + req: &http.Request{ + RemoteAddr: "[2001:db9::1]:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"2001:db8::1"}, }, }, want: "2001:db8::1, 2001:db9::1", }, { - name: "single forward address no RemoteAddr", - args: args{ - req: &http.Request{ - Header: map[string][]string{ - http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1"}, - }, + name: "single X-Forwarded-For address no RemoteAddr", + req: &http.Request{ + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"172.16.1.1"}, }, }, want: "172.16.1.1", }, { - name: "multiple forward with remote", - args: args{ - req: &http.Request{ - RemoteAddr: "192.168.1.100:3454", - Header: map[string][]string{ - http.CanonicalHeaderKey("X-FORWARDED-FOR"): {"172.16.1.1, 10.10.13.20"}, - }, + name: "multiple X-Forwarded-For with remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"172.16.1.1, 10.10.13.20"}, }, }, - want: "172.16.1.1, 10.10.13.20, 192.168.1.100", + want: "172.16.1.1, 192.168.1.100", }, { - name: "no forward header, no remote", - args: args{ - req: &http.Request{}, + name: "multiple X-Forwarded-For with IPv6 remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"[2001:db8:cafe::17]:4711, 10.10.13.20"}, + }, }, + want: "2001:db8:cafe::17, 192.168.1.100", + }, + { + name: "no header, no remote", + req: &http.Request{}, want: "", }, + { + name: "X-Real-IP with IPv6 remote with port", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xRealIP): {"[2001:db8:cafe::17]:4711"}, + }, + }, + want: "2001:db8:cafe::17, 192.168.1.100", + }, + { + name: "X-Real-IP with IPv4 remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xRealIP): {"192.169.1.200"}, + }, + }, + want: "192.169.1.200, 192.168.1.100", + }, + { + name: "X-Real-IP with IPv4 remote and X-Forwarded-For", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"[2001:db8:cafe::17]:4711, 10.10.13.20"}, + http.CanonicalHeaderKey(xRealIP): {"192.169.1.200"}, + }, + }, + want: "192.169.1.200, 192.168.1.100", + }, + { + name: "Forwarded with IPv4 remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(forwarded): {"for=192.169.1.200"}, + }, + }, + want: "192.169.1.200, 192.168.1.100", + }, + { + name: "Forwarded with IPv4 and proto and by fields", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(forwarded): {"for=192.0.2.60;proto=http;by=203.0.113.43"}, + }, + }, + want: "192.0.2.60, 192.168.1.100", + }, + { + name: "Forwarded with IPv6 and IPv4 remote", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(forwarded): {"for=[2001:db8:cafe::17]:4711,for=192.169.1.200"}, + }, + }, + want: "2001:db8:cafe::17, 192.168.1.100", + }, + { + name: "Forwarded with X-Real-IP and X-Forwarded-For", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"[2001:db8:cafe::17]:4711, 10.10.13.20"}, + http.CanonicalHeaderKey(xRealIP): {"192.169.1.200"}, + http.CanonicalHeaderKey(forwarded): {"for=[2001:db8:cafe::17]:4711,for=192.169.1.200"}, + }, + }, + want: "2001:db8:cafe::17, 192.168.1.100", + }, + { + name: "Forwarded returns hostname", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(forwarded): {"for=workstation.local"}, + }, + }, + want: "workstation.local, 192.168.1.100", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := GetSource(tt.args.req); got != tt.want { + if got := GetSource(tt.req); got != tt.want { t.Errorf("GetSource() = %v, want %v", got, tt.want) } }) diff --git a/middleware/logging.go b/middleware/logging.go index c494a49e..b2193134 100644 --- a/middleware/logging.go +++ b/middleware/logging.go @@ -19,18 +19,19 @@ type Log struct { // logWithRequest information from the request and context as fields. func (l Log) logWithRequest(r *http.Request) logging.Interface { + localLog := l.Log traceID, ok := ExtractTraceID(r.Context()) if ok { - l.Log = l.Log.WithField("traceID", traceID) + localLog = localLog.WithField("traceID", traceID) } // Add the source IPs derived from X-Forwarded-For and the request remote address sourceIPs := GetSource(r) if sourceIPs != "" { - l.Log = l.Log.WithField("sourceIPs", sourceIPs) + localLog = localLog.WithField("sourceIPs", sourceIPs) } - return user.LogWith(r.Context(), l.Log) + return user.LogWith(r.Context(), localLog) } // Wrap implements Middleware diff --git a/server/server_test.go b/server/server_test.go index 58a4bd73..1866e839 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -300,7 +300,7 @@ func TestMuxMiddleware(t *testing.T) { level.Set("info") cfg := Config{ HTTPListenAddress: "localhost", - HTTPListenPort: 9193, + HTTPListenPort: 9196, GRPCListenAddress: "localhost", HTTPMiddleware: []middleware.Interface{middleware.Logging}, MetricsNamespace: "testing_mux", @@ -317,7 +317,7 @@ func TestMuxMiddleware(t *testing.T) { go server.Run() defer server.Shutdown() - req, err := http.NewRequest("GET", "http://127.0.0.1:9192/error500", nil) + req, err := http.NewRequest("GET", "http://127.0.0.1:9196/error500", nil) require.NoError(t, err) http.DefaultClient.Do(req) } @@ -332,7 +332,7 @@ func TestTLSServer(t *testing.T) { cfg := Config{ HTTPListenAddress: "localhost", - HTTPListenPort: 9194, + HTTPListenPort: 9193, HTTPTLSConfig: node_https.TLSStruct{ TLSCertPath: "certs/server.crt", TLSKeyPath: "certs/server.key", @@ -347,7 +347,7 @@ func TestTLSServer(t *testing.T) { }, MetricsNamespace: "testing_tls", GRPCListenAddress: "localhost", - GRPCListenPort: 9195, + GRPCListenPort: 9194, } server, err := New(cfg) require.NoError(t, err) @@ -381,7 +381,7 @@ func TestTLSServer(t *testing.T) { } client := &http.Client{Transport: tr} - res, err := client.Get("https://localhost:9194/testhttps") + res, err := client.Get("https://localhost:9193/testhttps") require.NoError(t, err) defer res.Body.Close() @@ -392,7 +392,7 @@ func TestTLSServer(t *testing.T) { expected := []byte("Hello World!") require.Equal(t, expected, body) - conn, err := grpc.Dial("localhost:9195", grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) + conn, err := grpc.Dial("localhost:9194", grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) defer conn.Close() require.NoError(t, err) From d2b03221ceba1ef6a51de7fe788e25fc6819b048 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Wed, 12 Aug 2020 15:58:24 +0100 Subject: [PATCH 03/14] Revert go.mod changes Signed-off-by: Michel Hollands --- go.mod | 10 ++++++++++ go.sum | 1 - 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 31f45e35..d72adcda 100644 --- a/go.mod +++ b/go.mod @@ -12,15 +12,24 @@ require ( github.com/gogo/status v1.0.3 github.com/golang/lint v0.0.0-20180702182130-06c8688daad7 // indirect github.com/golang/protobuf v1.3.3 + github.com/gorilla/context v1.1.1 // indirect github.com/gorilla/mux v1.7.3 github.com/grpc-ecosystem/go-grpc-middleware v1.1.0 + github.com/konsorten/go-windows-terminal-sequences v1.0.1 // indirect + github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515 // indirect + github.com/mattn/go-colorable v0.0.9 // indirect + github.com/mattn/go-isatty v0.0.4 // indirect + github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b + github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + github.com/modern-go/reflect2 v1.0.1 // indirect github.com/opentracing-contrib/go-grpc v0.0.0-20180928155321-4b5a12d3ff02 github.com/opentracing-contrib/go-stdlib v0.0.0-20190519235532-cf7a6c988dc9 github.com/opentracing/opentracing-go v1.1.0 github.com/pkg/errors v0.9.1 github.com/pmezard/go-difflib v1.0.0 github.com/prometheus/client_golang v1.4.1 + github.com/prometheus/common v0.9.1 // indirect github.com/prometheus/node_exporter v1.0.0-rc.0.0.20200428091818-01054558c289 github.com/sercand/kuberesolver v2.1.0+incompatible github.com/sirupsen/logrus v1.4.2 @@ -30,6 +39,7 @@ require ( github.com/weaveworks/promrus v1.2.0 golang.org/x/net v0.0.0-20200202094626-16171245cfb2 golang.org/x/tools v0.0.0-20200216192241-b320d3a0f5a2 + google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55 // indirect google.golang.org/grpc v1.26.0 gopkg.in/yaml.v2 v2.2.8 ) diff --git a/go.sum b/go.sum index 80dbc10b..de35f58d 100644 --- a/go.sum +++ b/go.sum @@ -279,7 +279,6 @@ github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5Fsn github.com/prometheus/client_golang v1.3.0/go.mod h1:hJaj2vgQTGQmVCsAACORcieXFeDPbaTKGT+JTgUa3og= github.com/prometheus/client_golang v1.4.1 h1:FFSuS004yOQEtDdTq+TAOLP5xUq63KqAFYyOi8zA+Y8= github.com/prometheus/client_golang v1.4.1/go.mod h1:e9GMxYsXl05ICDXkRhurwBS4Q3OK1iX/F2sw+iXX5zU= -github.com/prometheus/client_golang v1.7.1 h1:NTGy1Ja9pByO+xAeH/qiWnLrKtr3hJPNjaVUwnjpdpA= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190115171406-56726106282f/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 h1:S/YWwWx/RA8rT8tKFRuGUZhuA90OyIBpPCXkcbwU8DE= From f69ab042fded8388b6dd2837161e4235bb320903 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 09:17:42 +0100 Subject: [PATCH 04/14] Remove making standard middleware optional Signed-off-by: Michel Hollands --- server/server.go | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/server/server.go b/server/server.go index 2516aadf..f7e8a819 100644 --- a/server/server.go +++ b/server/server.go @@ -242,12 +242,7 @@ func New(cfg Config) (*Server, error) { grpcServer := grpc.NewServer(grpcOptions...) // Setup HTTP server - var router *mux.Router - if cfg.Router != nil { - router = cfg.Router - } else { - router = mux.NewRouter() - } + router := mux.NewRouter() if cfg.PathPrefix != "" { // Expect metrics and pprof handlers to be prefixed with server's path prefix. // e.g. /loki/metrics or /loki/debug/pprof @@ -256,20 +251,17 @@ func New(cfg Config) (*Server, error) { if cfg.RegisterInstrumentation { RegisterInstrumentation(router) } - httpMiddleware := []middleware.Interface{} - if !cfg.DisableGeneratedHTTPMiddleware { - httpMiddleware = append(httpMiddleware, - middleware.Tracer{ - RouteMatcher: router, - }, - middleware.Log{ - Log: log, - }, - middleware.Instrument{ - Duration: requestDuration, - RouteMatcher: router, - }, - ) + httpMiddleware := []middleware.Interface{ + middleware.Tracer{ + RouteMatcher: router, + }, + middleware.Log{ + Log: log, + }, + middleware.Instrument{ + Duration: requestDuration, + RouteMatcher: router, + }, } httpMiddleware = append(httpMiddleware, cfg.HTTPMiddleware...) From a91cba9db389950d0369382aeb69f6ef25a52c3c Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 09:32:28 +0100 Subject: [PATCH 05/14] Remove unused fields as well Signed-off-by: Michel Hollands --- server/server.go | 10 ++++------ server/server_test.go | 2 -- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/server/server.go b/server/server.go index f7e8a819..4dc8ae0e 100644 --- a/server/server.go +++ b/server/server.go @@ -62,12 +62,10 @@ type Config struct { HTTPServerWriteTimeout time.Duration `yaml:"http_server_write_timeout"` HTTPServerIdleTimeout time.Duration `yaml:"http_server_idle_timeout"` - GRPCOptions []grpc.ServerOption `yaml:"-"` - GRPCMiddleware []grpc.UnaryServerInterceptor `yaml:"-"` - GRPCStreamMiddleware []grpc.StreamServerInterceptor `yaml:"-"` - HTTPMiddleware []middleware.Interface `yaml:"-"` - Router *mux.Router `yaml:"-"` - DisableGeneratedHTTPMiddleware bool `yaml:"-"` + GRPCOptions []grpc.ServerOption `yaml:"-"` + GRPCMiddleware []grpc.UnaryServerInterceptor `yaml:"-"` + GRPCStreamMiddleware []grpc.StreamServerInterceptor `yaml:"-"` + HTTPMiddleware []middleware.Interface `yaml:"-"` GPRCServerMaxRecvMsgSize int `yaml:"grpc_server_max_recv_msg_size"` GRPCServerMaxSendMsgSize int `yaml:"grpc_server_max_send_msg_size"` diff --git a/server/server_test.go b/server/server_test.go index 1866e839..dc3d67c9 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -17,7 +17,6 @@ import ( "google.golang.org/grpc/status" google_protobuf "github.com/golang/protobuf/ptypes/empty" - "github.com/gorilla/mux" "github.com/prometheus/client_golang/prometheus" node_https "github.com/prometheus/node_exporter/https" "github.com/stretchr/testify/require" @@ -305,7 +304,6 @@ func TestMuxMiddleware(t *testing.T) { HTTPMiddleware: []middleware.Interface{middleware.Logging}, MetricsNamespace: "testing_mux", LogLevel: level, - Router: mux.NewRouter(), } server, err := New(cfg) require.NoError(t, err) From c2ecf87d471aaf753c271a03d0f3bb255b9e6f40 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 09:52:12 +0100 Subject: [PATCH 06/14] Add option to turn logging of source IPs on Signed-off-by: Michel Hollands --- middleware/logging.go | 11 +++++---- server/server.go | 11 +++++---- server/server_test.go | 53 ++++++++++++++++++++++--------------------- 3 files changed, 41 insertions(+), 34 deletions(-) diff --git a/middleware/logging.go b/middleware/logging.go index b2193134..1d08198d 100644 --- a/middleware/logging.go +++ b/middleware/logging.go @@ -15,6 +15,7 @@ import ( type Log struct { Log logging.Interface LogRequestHeaders bool // LogRequestHeaders true -> dump http headers at debug log level + LogSourceIPs bool } // logWithRequest information from the request and context as fields. @@ -25,10 +26,12 @@ func (l Log) logWithRequest(r *http.Request) logging.Interface { localLog = localLog.WithField("traceID", traceID) } - // Add the source IPs derived from X-Forwarded-For and the request remote address - sourceIPs := GetSource(r) - if sourceIPs != "" { - localLog = localLog.WithField("sourceIPs", sourceIPs) + if l.LogSourceIPs { + // Add the source IPs derived from X-Forwarded-For and the request remote address + sourceIPs := GetSource(r) + if sourceIPs != "" { + localLog = localLog.WithField("sourceIPs", sourceIPs) + } } return user.LogWith(r.Context(), localLog) diff --git a/server/server.go b/server/server.go index 4dc8ae0e..3fc2f973 100644 --- a/server/server.go +++ b/server/server.go @@ -76,9 +76,10 @@ type Config struct { GRPCServerTime time.Duration `yaml:"grpc_server_keepalive_time"` GRPCServerTimeout time.Duration `yaml:"grpc_server_keepalive_timeout"` - LogFormat logging.Format `yaml:"log_format"` - LogLevel logging.Level `yaml:"log_level"` - Log logging.Interface `yaml:"-"` + LogFormat logging.Format `yaml:"log_format"` + LogLevel logging.Level `yaml:"log_level"` + Log logging.Interface `yaml:"-"` + LogSourceIPs bool `yaml:"log_source_ips"` // If not set, default signal handler is used. SignalHandler SignalHandler `yaml:"-"` @@ -120,6 +121,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.PathPrefix, "server.path-prefix", "", "Base path to serve all API routes from (e.g. /v1/)") cfg.LogFormat.RegisterFlags(f) cfg.LogLevel.RegisterFlags(f) + f.BoolVar(&cfg.LogSourceIPs, "log.log-source-ips", false, "Log the source IPs") } // Server wraps a HTTP and gRPC server, and some common initialization. @@ -254,7 +256,8 @@ func New(cfg Config) (*Server, error) { RouteMatcher: router, }, middleware.Log{ - Log: log, + Log: log, + LogSourceIPs: cfg.LogSourceIPs, }, middleware.Instrument{ Duration: requestDuration, diff --git a/server/server_test.go b/server/server_test.go index dc3d67c9..d0f1b85a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -294,32 +294,6 @@ func TestMiddlewareLogging(t *testing.T) { http.DefaultClient.Do(req) } -func TestMuxMiddleware(t *testing.T) { - var level logging.Level - level.Set("info") - cfg := Config{ - HTTPListenAddress: "localhost", - HTTPListenPort: 9196, - GRPCListenAddress: "localhost", - HTTPMiddleware: []middleware.Interface{middleware.Logging}, - MetricsNamespace: "testing_mux", - LogLevel: level, - } - server, err := New(cfg) - require.NoError(t, err) - - server.HTTP.HandleFunc("/error500", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(500) - }) - - go server.Run() - defer server.Shutdown() - - req, err := http.NewRequest("GET", "http://127.0.0.1:9196/error500", nil) - require.NoError(t, err) - http.DefaultClient.Do(req) -} - func TestTLSServer(t *testing.T) { var level logging.Level level.Set("info") @@ -401,6 +375,33 @@ func TestTLSServer(t *testing.T) { require.EqualValues(t, &empty, grpcRes) } +func TestLogSourceIPs(t *testing.T) { + var level logging.Level + level.Set("debug") + cfg := Config{ + HTTPListenAddress: "localhost", + HTTPListenPort: 9195, + GRPCListenAddress: "localhost", + HTTPMiddleware: []middleware.Interface{middleware.Logging}, + MetricsNamespace: "testing_mux", + LogLevel: level, + LogSourceIPs: true, + } + server, err := New(cfg) + require.NoError(t, err) + + server.HTTP.HandleFunc("/error500", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + }) + + go server.Run() + defer server.Shutdown() + + req, err := http.NewRequest("GET", "http://127.0.0.1:9195/error500", nil) + require.NoError(t, err) + http.DefaultClient.Do(req) +} + func TestStopWithDisabledSignalHandling(t *testing.T) { cfg := Config{ HTTPListenAddress: "localhost", From 2ad2d89402ff82164380502fc957afc4fc3da455 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 10:32:59 +0100 Subject: [PATCH 07/14] Add sourceIPs tag in tracing Signed-off-by: Michel Hollands --- middleware/http_tracing.go | 26 +++++++++++++++++--------- server/server.go | 1 + 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/middleware/http_tracing.go b/middleware/http_tracing.go index 54c99844..a1b97bd0 100644 --- a/middleware/http_tracing.go +++ b/middleware/http_tracing.go @@ -17,20 +17,28 @@ var _ = nethttp.MWURLTagFunc // Tracer is a middleware which traces incoming requests. type Tracer struct { RouteMatcher RouteMatcher + LogSourceIPs bool } // Wrap implements Interface func (t Tracer) Wrap(next http.Handler) http.Handler { - opMatcher := nethttp.OperationNameFunc(func(r *http.Request) string { - op := getRouteName(t.RouteMatcher, r) - if op == "" { - return "HTTP " + r.Method - } - - return fmt.Sprintf("HTTP %s - %s", r.Method, op) - }) + options := []nethttp.MWOption{ + nethttp.OperationNameFunc(func(r *http.Request) string { + op := getRouteName(t.RouteMatcher, r) + if op == "" { + return "HTTP " + r.Method + } + + return fmt.Sprintf("HTTP %s - %s", r.Method, op) + }), + } + if t.LogSourceIPs { + options = append(options, nethttp.MWSpanObserver(func(sp opentracing.Span, r *http.Request) { + sp.SetTag("sourceIPs", GetSource(r)) + })) + } - return nethttp.Middleware(opentracing.GlobalTracer(), next, opMatcher) + return nethttp.Middleware(opentracing.GlobalTracer(), next, options...) } // ExtractTraceID extracts the trace id, if any from the context. diff --git a/server/server.go b/server/server.go index 3fc2f973..2c3f5fbb 100644 --- a/server/server.go +++ b/server/server.go @@ -254,6 +254,7 @@ func New(cfg Config) (*Server, error) { httpMiddleware := []middleware.Interface{ middleware.Tracer{ RouteMatcher: router, + LogSourceIPs: cfg.LogSourceIPs, }, middleware.Log{ Log: log, From bc7566749df6a54feed0c0dff0562f92575311d7 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 13:27:38 +0100 Subject: [PATCH 08/14] Add custom regex for extracting IP from header Signed-off-by: Michel Hollands --- middleware/forwarded.go | 53 +++++++++++++++++++++++--- middleware/forwarded_test.go | 74 +++++++++++++++++++++++++++++++++++- middleware/http_tracing.go | 6 +-- middleware/logging.go | 11 +++--- server/server.go | 25 ++++++++---- 5 files changed, 147 insertions(+), 22 deletions(-) diff --git a/middleware/forwarded.go b/middleware/forwarded.go index 2d89d6fd..75f3dadb 100644 --- a/middleware/forwarded.go +++ b/middleware/forwarded.go @@ -32,6 +32,31 @@ var ( protoRegex = regexp.MustCompile(`(?i)(?:proto=)(https|http)`) ) +// SourceIPExtractor extracts the source IPs from a HTTP request +type SourceIPExtractor struct { + // The header to search for + header string + // A regex that extracts the IP address from the header. + // It should contain at least one capturing group the first of which will be returned. + regex *regexp.Regexp +} + +// NewSourceIPs creates a new SourceIPs +func NewSourceIPs(header, regex string) (*SourceIPExtractor, error) { + if (header == "" && regex != "") || (header != "" && regex == "") { + return nil, fmt.Errorf("either both a header field and a regex have to be given or neither") + } + re, err := regexp.Compile(regex) + if err != nil { + return nil, fmt.Errorf("invalid regex given") + } + + return &SourceIPExtractor{ + header: header, + regex: re, + }, nil +} + // extractHost returns the Host IP address without any port information func extractHost(address string) string { hostIP := net.ParseIP(address) @@ -47,9 +72,9 @@ func extractHost(address string) string { return hostStr } -// GetSource returns any source addresses we can find in the request, comma-separated -func GetSource(req *http.Request) string { - fwd := extractHost(getIP(req)) +// Get returns any source addresses we can find in the request, comma-separated +func (sips SourceIPExtractor) Get(req *http.Request) string { + fwd := extractHost(sips.getIP(req)) if fwd == "" { if req.RemoteAddr == "" { return "" @@ -69,10 +94,28 @@ func GetSource(req *http.Request) string { } // getIP retrieves the IP from the RFC7239 Forwarded headers, -// X-Real-IP and X-Forwarded-For (in that order). -func getIP(r *http.Request) string { +// X-Real-IP and X-Forwarded-For (in that order) or from the +// custom regex. +func (sips SourceIPExtractor) getIP(r *http.Request) string { var addr string + if sips.header != "" { + hdr := r.Header.Get(sips.header) + if hdr == "" { + return "" + } + allMatches := sips.regex.FindAllStringSubmatch(hdr, 1) + if allMatches == nil { + return "" + } + firstMatch := allMatches[0] + // Check there is at least 1 submatch + if len(firstMatch) < 2 { + return "" + } + return firstMatch[1] + } + if fwd := r.Header.Get(forwarded); fwd != "" { // match should contain at least two elements if the protocol was // specified in the Forwarded header. The first element will always be diff --git a/middleware/forwarded_test.go b/middleware/forwarded_test.go index 7586c4fc..a545f7a3 100644 --- a/middleware/forwarded_test.go +++ b/middleware/forwarded_test.go @@ -3,6 +3,8 @@ package middleware import ( "net/http" "testing" + + "github.com/stretchr/testify/require" ) func TestGetSource(t *testing.T) { @@ -176,9 +178,79 @@ func TestGetSource(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := GetSource(tt.req); got != tt.want { + sourceIPs, err := NewSourceIPs("", "") + require.NoError(t, err) + + if got := sourceIPs.Get(tt.req); got != tt.want { t.Errorf("GetSource() = %v, want %v", got, tt.want) } }) } } + +func TestGetSourceWithCustomRegex(t *testing.T) { + tests := []struct { + name string + req *http.Request + want string + }{ + { + name: "no header", + req: &http.Request{RemoteAddr: "192.168.1.100:3454"}, + want: "192.168.1.100", + }, + { + name: "No matching entry in the header", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("SomeHeader"): {"not matching"}, + }, + }, + want: "192.168.1.100", + }, + { + name: "one matching entry in the header", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("SomeHeader"): {"172.16.1.1"}, + }, + }, + want: "172.16.1.1, 192.168.1.100", + }, + { + name: "multiple matching entries in the header, only first used", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey("SomeHeader"): {"172.16.1.1", "172.16.2.1"}, + }, + }, + want: "172.16.1.1, 192.168.1.100", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sourceIPs, err := NewSourceIPs("SomeHeader", "((?:[0-9]{1,3}\\.){3}[0-9]{1,3})") + require.NoError(t, err) + + if got := sourceIPs.Get(tt.req); got != tt.want { + t.Errorf("GetSource() = %v, want %v", got, tt.want) + } + }) + } +} +func TestInvalidCustomRegex(t *testing.T) { + sourceIPs, err := NewSourceIPs("Header", "") + require.Empty(t, sourceIPs) + require.Error(t, err) + + sourceIPs, err = NewSourceIPs("", "a(.*)b") + require.Empty(t, sourceIPs) + require.Error(t, err) + + sourceIPs, err = NewSourceIPs("Header", "[*") + require.Empty(t, sourceIPs) + require.Error(t, err) +} diff --git a/middleware/http_tracing.go b/middleware/http_tracing.go index a1b97bd0..a1ac40f5 100644 --- a/middleware/http_tracing.go +++ b/middleware/http_tracing.go @@ -17,7 +17,7 @@ var _ = nethttp.MWURLTagFunc // Tracer is a middleware which traces incoming requests. type Tracer struct { RouteMatcher RouteMatcher - LogSourceIPs bool + SourceIPs *SourceIPExtractor } // Wrap implements Interface @@ -32,9 +32,9 @@ func (t Tracer) Wrap(next http.Handler) http.Handler { return fmt.Sprintf("HTTP %s - %s", r.Method, op) }), } - if t.LogSourceIPs { + if t.SourceIPs != nil { options = append(options, nethttp.MWSpanObserver(func(sp opentracing.Span, r *http.Request) { - sp.SetTag("sourceIPs", GetSource(r)) + sp.SetTag("sourceIPs", t.SourceIPs.Get(r)) })) } diff --git a/middleware/logging.go b/middleware/logging.go index 1d08198d..00270df9 100644 --- a/middleware/logging.go +++ b/middleware/logging.go @@ -15,7 +15,7 @@ import ( type Log struct { Log logging.Interface LogRequestHeaders bool // LogRequestHeaders true -> dump http headers at debug log level - LogSourceIPs bool + SourceIPs *SourceIPExtractor } // logWithRequest information from the request and context as fields. @@ -26,11 +26,10 @@ func (l Log) logWithRequest(r *http.Request) logging.Interface { localLog = localLog.WithField("traceID", traceID) } - if l.LogSourceIPs { - // Add the source IPs derived from X-Forwarded-For and the request remote address - sourceIPs := GetSource(r) - if sourceIPs != "" { - localLog = localLog.WithField("sourceIPs", sourceIPs) + if l.SourceIPs != nil { + ips := l.SourceIPs.Get(r) + if ips != "" { + localLog = localLog.WithField("sourceIPs", ips) } } diff --git a/server/server.go b/server/server.go index 2c3f5fbb..0e2519b2 100644 --- a/server/server.go +++ b/server/server.go @@ -76,10 +76,12 @@ type Config struct { GRPCServerTime time.Duration `yaml:"grpc_server_keepalive_time"` GRPCServerTimeout time.Duration `yaml:"grpc_server_keepalive_timeout"` - LogFormat logging.Format `yaml:"log_format"` - LogLevel logging.Level `yaml:"log_level"` - Log logging.Interface `yaml:"-"` - LogSourceIPs bool `yaml:"log_source_ips"` + LogFormat logging.Format `yaml:"log_format"` + LogLevel logging.Level `yaml:"log_level"` + Log logging.Interface `yaml:"-"` + LogSourceIPs bool `yaml:"log_source_ips"` + LogSourceIPsHeader string `yaml:"log_source_ips_header"` + LogSourceIPsRegex string `yaml:"log_source_ips_regex"` // If not set, default signal handler is used. SignalHandler SignalHandler `yaml:"-"` @@ -122,6 +124,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { cfg.LogFormat.RegisterFlags(f) cfg.LogLevel.RegisterFlags(f) f.BoolVar(&cfg.LogSourceIPs, "log.log-source-ips", false, "Log the source IPs") + f.StringVar(&cfg.LogSourceIPsHeader, "log.log-source-ips-header", "", "Header field storing the source IPs") + f.StringVar(&cfg.LogSourceIPsRegex, "log.log-source-ips-regex", "", "Regex for matching the source IPs") } // Server wraps a HTTP and gRPC server, and some common initialization. @@ -251,14 +255,21 @@ func New(cfg Config) (*Server, error) { if cfg.RegisterInstrumentation { RegisterInstrumentation(router) } + var sourceIPs *middleware.SourceIPExtractor + if cfg.LogSourceIPs { + sourceIPs, err = middleware.NewSourceIPs(cfg.LogSourceIPsHeader, cfg.LogSourceIPsRegex) + if err != nil { + return nil, fmt.Errorf("error setting up source IP extraction: %v", err) + } + } httpMiddleware := []middleware.Interface{ middleware.Tracer{ RouteMatcher: router, - LogSourceIPs: cfg.LogSourceIPs, + SourceIPs: sourceIPs, }, middleware.Log{ - Log: log, - LogSourceIPs: cfg.LogSourceIPs, + Log: log, + SourceIPs: sourceIPs, }, middleware.Instrument{ Duration: requestDuration, From d47f0042ce36fb1da2bd551507528e3aabbc4943 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 13:42:54 +0100 Subject: [PATCH 09/14] Rename file so it's clearer what it does Signed-off-by: Michel Hollands --- middleware/{forwarded.go => source_ips.go} | 0 middleware/{forwarded_test.go => source_ips_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename middleware/{forwarded.go => source_ips.go} (100%) rename middleware/{forwarded_test.go => source_ips_test.go} (100%) diff --git a/middleware/forwarded.go b/middleware/source_ips.go similarity index 100% rename from middleware/forwarded.go rename to middleware/source_ips.go diff --git a/middleware/forwarded_test.go b/middleware/source_ips_test.go similarity index 100% rename from middleware/forwarded_test.go rename to middleware/source_ips_test.go From 03477c76fa1166e99043bcd6cf02444c4d9f9335 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Thu, 13 Aug 2020 13:47:07 +0100 Subject: [PATCH 10/14] Use better names Signed-off-by: Michel Hollands --- middleware/source_ips.go | 1 + middleware/source_ips_test.go | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/middleware/source_ips.go b/middleware/source_ips.go index 75f3dadb..6dfdd117 100644 --- a/middleware/source_ips.go +++ b/middleware/source_ips.go @@ -99,6 +99,7 @@ func (sips SourceIPExtractor) Get(req *http.Request) string { func (sips SourceIPExtractor) getIP(r *http.Request) string { var addr string + // Use the custom regex only if it was setup if sips.header != "" { hdr := r.Header.Get(sips.header) if hdr == "" { diff --git a/middleware/source_ips_test.go b/middleware/source_ips_test.go index a545f7a3..859d3e81 100644 --- a/middleware/source_ips_test.go +++ b/middleware/source_ips_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestGetSource(t *testing.T) { +func TestGetSourceIPs(t *testing.T) { tests := []struct { name string req *http.Request @@ -188,7 +188,7 @@ func TestGetSource(t *testing.T) { } } -func TestGetSourceWithCustomRegex(t *testing.T) { +func TestGetSourceIPsWithCustomRegex(t *testing.T) { tests := []struct { name string req *http.Request @@ -241,7 +241,7 @@ func TestGetSourceWithCustomRegex(t *testing.T) { }) } } -func TestInvalidCustomRegex(t *testing.T) { +func TestInvalid(t *testing.T) { sourceIPs, err := NewSourceIPs("Header", "") require.Empty(t, sourceIPs) require.Error(t, err) From 8069698b1d0a6728adc421ad3acd730da9273e90 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Fri, 14 Aug 2020 09:03:01 +0100 Subject: [PATCH 11/14] Do no use log prefix in config var name Signed-off-by: Michel Hollands --- server/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/server.go b/server/server.go index 0e2519b2..0579488f 100644 --- a/server/server.go +++ b/server/server.go @@ -123,9 +123,9 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.PathPrefix, "server.path-prefix", "", "Base path to serve all API routes from (e.g. /v1/)") cfg.LogFormat.RegisterFlags(f) cfg.LogLevel.RegisterFlags(f) - f.BoolVar(&cfg.LogSourceIPs, "log.log-source-ips", false, "Log the source IPs") - f.StringVar(&cfg.LogSourceIPsHeader, "log.log-source-ips-header", "", "Header field storing the source IPs") - f.StringVar(&cfg.LogSourceIPsRegex, "log.log-source-ips-regex", "", "Regex for matching the source IPs") + f.BoolVar(&cfg.LogSourceIPs, "server.log-source-ips", false, "Log the source IPs") + f.StringVar(&cfg.LogSourceIPsHeader, "server.log-source-ips-header", "", "Header field storing the source IPs") + f.StringVar(&cfg.LogSourceIPsRegex, "server.log-source-ips-regex", "", "Regex for matching the source IPs") } // Server wraps a HTTP and gRPC server, and some common initialization. From f6c114774ebe2a5d59feb2bc9c0dfe8e6125614b Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Fri, 14 Aug 2020 17:05:15 +0100 Subject: [PATCH 12/14] Address review comments Signed-off-by: Michel Hollands --- middleware/source_ips.go | 16 +++++----------- middleware/source_ips_test.go | 10 ++++++++++ server/server.go | 8 ++++---- server/server_test.go | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/middleware/source_ips.go b/middleware/source_ips.go index 6dfdd117..17178d42 100644 --- a/middleware/source_ips.go +++ b/middleware/source_ips.go @@ -12,11 +12,8 @@ import ( var ( // De-facto standard header keys. - xForwardedFor = http.CanonicalHeaderKey("X-Forwarded-For") - xForwardedHost = http.CanonicalHeaderKey("X-Forwarded-Host") - xForwardedProto = http.CanonicalHeaderKey("X-Forwarded-Proto") - xForwardedScheme = http.CanonicalHeaderKey("X-Forwarded-Scheme") - xRealIP = http.CanonicalHeaderKey("X-Real-IP") + xForwardedFor = http.CanonicalHeaderKey("X-Forwarded-For") + xRealIP = http.CanonicalHeaderKey("X-Real-IP") ) var ( @@ -27,9 +24,6 @@ var ( // Allows for a sub-match of the first value after 'for=' to the next // comma, semi-colon or space. The match is case-insensitive. forRegex = regexp.MustCompile(`(?i)(?:for=)([^(;|,| )]+)`) - // Allows for a sub-match for the first instance of scheme (http|https) - // prefixed by 'proto='. The match is case-insensitive. - protoRegex = regexp.MustCompile(`(?i)(?:proto=)(https|http)`) ) // SourceIPExtractor extracts the source IPs from a HTTP request @@ -106,7 +100,7 @@ func (sips SourceIPExtractor) getIP(r *http.Request) string { return "" } allMatches := sips.regex.FindAllStringSubmatch(hdr, 1) - if allMatches == nil { + if len(allMatches) == 0 { return "" } firstMatch := allMatches[0] @@ -132,11 +126,11 @@ func (sips SourceIPExtractor) getIP(r *http.Request) string { // X-Real-IP should only contain one IP address (the client making the // request). addr = fwd - } else if fwd := r.Header.Get(xForwardedFor); fwd != "" { + } else if fwd := strings.ReplaceAll(r.Header.Get(xForwardedFor), " ", ""); fwd != "" { // Only grab the first (client) address. Note that '192.168.0.1, // 10.1.1.1' is a valid key for X-Forwarded-For where addresses after // the first may represent forwarding proxies earlier in the chain. - s := strings.Index(fwd, ", ") + s := strings.Index(fwd, ",") if s == -1 { s = len(fwd) } diff --git a/middleware/source_ips_test.go b/middleware/source_ips_test.go index 859d3e81..455a5383 100644 --- a/middleware/source_ips_test.go +++ b/middleware/source_ips_test.go @@ -77,6 +77,16 @@ func TestGetSourceIPs(t *testing.T) { }, want: "172.16.1.1, 192.168.1.100", }, + { + name: "multiple X-Forwarded-For with remote and no spaces", + req: &http.Request{ + RemoteAddr: "192.168.1.100:3454", + Header: map[string][]string{ + http.CanonicalHeaderKey(xForwardedFor): {"172.16.1.1,10.10.13.20,10.11.16.46"}, + }, + }, + want: "172.16.1.1, 192.168.1.100", + }, { name: "multiple X-Forwarded-For with IPv6 remote", req: &http.Request{ diff --git a/server/server.go b/server/server.go index 0579488f..cf874936 100644 --- a/server/server.go +++ b/server/server.go @@ -79,7 +79,7 @@ type Config struct { LogFormat logging.Format `yaml:"log_format"` LogLevel logging.Level `yaml:"log_level"` Log logging.Interface `yaml:"-"` - LogSourceIPs bool `yaml:"log_source_ips"` + LogSourceIPs bool `yaml:"log_source_ips_enabled"` LogSourceIPsHeader string `yaml:"log_source_ips_header"` LogSourceIPsRegex string `yaml:"log_source_ips_regex"` @@ -123,9 +123,9 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.PathPrefix, "server.path-prefix", "", "Base path to serve all API routes from (e.g. /v1/)") cfg.LogFormat.RegisterFlags(f) cfg.LogLevel.RegisterFlags(f) - f.BoolVar(&cfg.LogSourceIPs, "server.log-source-ips", false, "Log the source IPs") - f.StringVar(&cfg.LogSourceIPsHeader, "server.log-source-ips-header", "", "Header field storing the source IPs") - f.StringVar(&cfg.LogSourceIPsRegex, "server.log-source-ips-regex", "", "Regex for matching the source IPs") + f.BoolVar(&cfg.LogSourceIPs, "server.log-source-ips-enabled", false, "Optionally log the source IPs. Default: false") + f.StringVar(&cfg.LogSourceIPsHeader, "server.log-source-ips-header", "", "Header field storing the source IPs. Only used if server.log-source-ips-enabled is true. If not set the default Forwarded, X-Real-IP and X-Forwarded-For headers are used") + f.StringVar(&cfg.LogSourceIPsRegex, "server.log-source-ips-regex", "", "Regex for matching the source IPs. Only used if server.log-source-ips-enabled is true. If not set the default Forwarded, X-Real-IP and X-Forwarded-For headers are used") } // Server wraps a HTTP and gRPC server, and some common initialization. diff --git a/server/server_test.go b/server/server_test.go index d0f1b85a..92193661 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -375,9 +375,38 @@ func TestTLSServer(t *testing.T) { require.EqualValues(t, &empty, grpcRes) } +type FakeLogger struct { + sourceIPs string +} + +func (f FakeLogger) Debugf(format string, args ...interface{}) {} +func (f FakeLogger) Debugln(args ...interface{}) {} + +func (f FakeLogger) Infof(format string, args ...interface{}) {} +func (f FakeLogger) Infoln(args ...interface{}) {} + +func (f FakeLogger) Errorf(format string, args ...interface{}) {} +func (f FakeLogger) Errorln(args ...interface{}) {} + +func (f FakeLogger) Warnf(format string, args ...interface{}) {} +func (f FakeLogger) Warnln(args ...interface{}) {} + +func (f *FakeLogger) WithField(key string, value interface{}) logging.Interface { + if key == "sourceIPs" { + f.sourceIPs = value.(string) + } + + return f +} + +func (f *FakeLogger) WithFields(fields logging.Fields) logging.Interface { + return f +} + func TestLogSourceIPs(t *testing.T) { var level logging.Level level.Set("debug") + fake := FakeLogger{} cfg := Config{ HTTPListenAddress: "localhost", HTTPListenPort: 9195, @@ -385,6 +414,7 @@ func TestLogSourceIPs(t *testing.T) { HTTPMiddleware: []middleware.Interface{middleware.Logging}, MetricsNamespace: "testing_mux", LogLevel: level, + Log: &fake, LogSourceIPs: true, } server, err := New(cfg) @@ -397,9 +427,13 @@ func TestLogSourceIPs(t *testing.T) { go server.Run() defer server.Shutdown() + require.Empty(t, fake.sourceIPs) + req, err := http.NewRequest("GET", "http://127.0.0.1:9195/error500", nil) require.NoError(t, err) http.DefaultClient.Do(req) + + require.NotEmpty(t, fake.sourceIPs) } func TestStopWithDisabledSignalHandling(t *testing.T) { From 2c7a6ca78098696bc633d90d72323926bd42f43e Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Mon, 17 Aug 2020 09:40:43 +0100 Subject: [PATCH 13/14] Use same way of access struct Signed-off-by: Michel Hollands --- server/server_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/server/server_test.go b/server/server_test.go index 92193661..dbf1481e 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -379,17 +379,17 @@ type FakeLogger struct { sourceIPs string } -func (f FakeLogger) Debugf(format string, args ...interface{}) {} -func (f FakeLogger) Debugln(args ...interface{}) {} +func (f *FakeLogger) Debugf(format string, args ...interface{}) {} +func (f *FakeLogger) Debugln(args ...interface{}) {} -func (f FakeLogger) Infof(format string, args ...interface{}) {} -func (f FakeLogger) Infoln(args ...interface{}) {} +func (f *FakeLogger) Infof(format string, args ...interface{}) {} +func (f *FakeLogger) Infoln(args ...interface{}) {} -func (f FakeLogger) Errorf(format string, args ...interface{}) {} -func (f FakeLogger) Errorln(args ...interface{}) {} +func (f *FakeLogger) Errorf(format string, args ...interface{}) {} +func (f *FakeLogger) Errorln(args ...interface{}) {} -func (f FakeLogger) Warnf(format string, args ...interface{}) {} -func (f FakeLogger) Warnln(args ...interface{}) {} +func (f *FakeLogger) Warnf(format string, args ...interface{}) {} +func (f *FakeLogger) Warnln(args ...interface{}) {} func (f *FakeLogger) WithField(key string, value interface{}) logging.Interface { if key == "sourceIPs" { From 7900f78079b45da1eb28ecb1c577a1d94157a1d3 Mon Sep 17 00:00:00 2001 From: Michel Hollands Date: Mon, 17 Aug 2020 10:57:06 +0100 Subject: [PATCH 14/14] Address review comments Signed-off-by: Michel Hollands --- server/server.go | 2 +- server/server_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/server.go b/server/server.go index cf874936..c8c44d98 100644 --- a/server/server.go +++ b/server/server.go @@ -123,7 +123,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.PathPrefix, "server.path-prefix", "", "Base path to serve all API routes from (e.g. /v1/)") cfg.LogFormat.RegisterFlags(f) cfg.LogLevel.RegisterFlags(f) - f.BoolVar(&cfg.LogSourceIPs, "server.log-source-ips-enabled", false, "Optionally log the source IPs. Default: false") + f.BoolVar(&cfg.LogSourceIPs, "server.log-source-ips-enabled", false, "Optionally log the source IPs.") f.StringVar(&cfg.LogSourceIPsHeader, "server.log-source-ips-header", "", "Header field storing the source IPs. Only used if server.log-source-ips-enabled is true. If not set the default Forwarded, X-Real-IP and X-Forwarded-For headers are used") f.StringVar(&cfg.LogSourceIPsRegex, "server.log-source-ips-regex", "", "Regex for matching the source IPs. Only used if server.log-source-ips-enabled is true. If not set the default Forwarded, X-Real-IP and X-Forwarded-For headers are used") } diff --git a/server/server_test.go b/server/server_test.go index dbf1481e..a4f8add9 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -433,7 +433,7 @@ func TestLogSourceIPs(t *testing.T) { require.NoError(t, err) http.DefaultClient.Do(req) - require.NotEmpty(t, fake.sourceIPs) + require.Equal(t, fake.sourceIPs, "127.0.0.1") } func TestStopWithDisabledSignalHandling(t *testing.T) {