Merge pull request #7 from snappyflow/rum-ip-saas-fix-7534

[SNP-7534] Fix incorrect IP address in SaaS mode

beater/middleware/request_metadata_middleware.go

@@ -29,7 +29,13 @@ func UserMetadataMiddleware() Middleware {
 		return func(c *request.Context) {
 			dec := utility.ManualDecoder{}
 			c.RequestMetadata.UserAgent = dec.UserAgentHeader(c.Request.Header)
-			c.RequestMetadata.ClientIP = utility.ExtractIP(c.Request)
+
+			if c.IsRum {
+				c.RequestMetadata.ClientIP = utility.ExtractIPRUM(c.Request)
+			} else {
+				c.RequestMetadata.ClientIP = utility.ExtractIP(c.Request)
+			}
+
 			h(c)
 		}, nil
 	}

model/modeldecoder/context.go

@@ -76,7 +76,7 @@ func decodeContext(input map[string]interface{}, cfg Config, meta *model.Metadat
 		meta.UserAgent.Original = ua
 	}
 	if meta.Client.IP == nil {
-		meta.Client.IP = getHTTPClientIP(http)
+		meta.Client.IP = getHTTPClientIP(http, ctx.Labels)
 	}
 
 	if serviceInp := getObject(input, fieldName("service")); serviceInp != nil {
@@ -124,16 +124,22 @@ func decodeURL(raw common.MapStr, err error) (*model.URL, error) {
 	return &url, err
 }
 
-func getHTTPClientIP(http *model.Http) net.IP {
+func getHTTPClientIP(http *model.Http, labels *model.Labels) net.IP {
 	if http == nil || http.Request == nil {
 		return nil
 	}
 	// http.Request.Headers and http.Request.Socket information is
 	// only set for backend events try to first extract an IP address
 	// from the headers, if not possible use IP address from socket
 	// remote_address
-	if ip := utility.ExtractIPFromHeader(http.Request.Headers); ip != nil {
-		return ip
+	if agent, _ := labels.Fields().GetValue("_tag_agent"); agent == "rum" {
+		if ip := utility.ExtractIPFromHeaderRum(http.Request.Headers); ip != nil {
+			return ip
+		}
+	} else {
+		if ip := utility.ExtractIPFromHeader(http.Request.Headers); ip != nil {
+			return ip
+		}
 	}
 	if http.Request.Socket != nil && http.Request.Socket.RemoteAddress != nil {
 		return utility.ParseIP(*http.Request.Socket.RemoteAddress)

utility/ip.go

@@ -31,6 +31,14 @@ func ExtractIP(r *http.Request) net.IP {
 	return ParseIP(r.RemoteAddr)
 }
 
+func ExtractIPRUM(r *http.Request) net.IP {
+	if ip := ExtractIPFromHeaderRum(r.Header); ip != nil {
+		return ip
+	}
+
+	return ParseIP((r.RemoteAddr))
+}
+
 // ExtractIPFromHeader extracts host information from `Forwarded`, `X-Real-IP`, `X-Forwarded-For` headers,
 // in this order. The first valid IP address extracted is returned.
 func ExtractIPFromHeader(header http.Header) net.IP {
@@ -42,6 +50,18 @@ func ExtractIPFromHeader(header http.Header) net.IP {
 	return nil
 }
 
+// Identical to ExtractIPFromHeader, but also extracts host information from `X-Original-Forwarded-For` header,
+// with that header taking priority over all others present in the request body.
+func ExtractIPFromHeaderRum(header http.Header) net.IP {
+	for _, parseFn := range parseRumHeadersInOrder {
+		if ip := ParseIP(parseFn(header)); ip != nil {
+			return ip
+		}
+	}
+
+	return nil
+}
+
 // ParseIP returns the IP address parsed from a given input if a valid IP can be extracted. Otherwise returns nil.
 func ParseIP(inp string) net.IP {
 	if inp == "" {

utility/ip_test.go

@@ -30,9 +30,10 @@ import (
 )
 
 const (
-	headerForwarded     = "Forwarded"
-	headerXForwardedFor = "X-Forwarded-For"
-	headerXRealIP       = "X-Real-IP"
+	headerForwarded             = "Forwarded"
+	headerXForwardedFor         = "X-Forwarded-For"
+	headerXRealIP               = "X-Real-IP"
+	headerXOriginalForwardedFor = "X-Original-Forwarded-For"
 )
 
 func TestExtractIP(t *testing.T) {
@@ -58,6 +59,39 @@ func TestExtractIP(t *testing.T) {
 	assert.Equal(t, "2001:db8:cafe::17", utility.ExtractIP(req).String())
 }
 
+func TestExtractIPRUM(t *testing.T) {
+	assert.Nil(t, utility.ExtractIPRUM(&http.Request{}))
+	assert.Nil(t, utility.ExtractIPRUM(&http.Request{RemoteAddr: "invalid"}))
+
+	//	Direct
+	assert.Equal(t, "::1", utility.ExtractIPRUM(&http.Request{RemoteAddr: "[::1]:1234"}).String())
+	assert.Equal(t, "192.168.0.1", utility.ExtractIPRUM(&http.Request{RemoteAddr: "192.168.0.1"}).String())
+
+	req := &http.Request{
+		RemoteAddr: "[::1]:1234",
+		Header:     make(http.Header),
+	}
+
+	//	X-Forwarded-For
+	req.Header.Set(headerXForwardedFor, "client.invalid")
+	assert.Equal(t, "::1", utility.ExtractIPRUM(req).String())
+
+	//	X-Real-IP should override X-Forwarded-For
+	req.Header.Set(headerXRealIP, "127.1.2.3")
+	assert.Equal(t, "127.1.2.3", utility.ExtractIPRUM(req).String())
+
+	//	Forwarded header should override X-Forwarded-For and X-Real-IP
+	req.Header.Set(headerForwarded, "for=_secret") //	invalid, fall back
+	assert.Equal(t, "127.1.2.3", utility.ExtractIPRUM(req).String())
+
+	req.Header.Set(headerForwarded, "for=[2001:db8:cafe::17]:4711")
+	assert.Equal(t, "2001:db8:cafe::17", utility.ExtractIPRUM(req).String())
+
+	//	X-Original-Forwarded-For should override all others
+	req.Header.Set(headerXOriginalForwardedFor, "38.60.220.1")
+	assert.Equal(t, "38.60.220.1", utility.ExtractIPRUM(req).String())
+}
+
 func TestExtractIPFromHeader(t *testing.T) {
 	for name, tc := range map[string]struct {
 		header map[string]string
@@ -106,6 +140,58 @@ func TestExtractIPFromHeader(t *testing.T) {
 	}
 }
 
+func TestExtractIPFromHeaderRum(t *testing.T) {
+	for name, tc := range map[string]struct {
+		header map[string]string
+		ip     string
+	}{
+		"no header":                        {},
+		"Invalid-X-Forwarded-For":          {header: map[string]string{headerXForwardedFor: "client.invalid"}},
+		"Invalid-X-Real-IP-Invalid":        {header: map[string]string{headerXRealIP: "client.invalid"}},
+		"Invalid-Forwarded":                {header: map[string]string{headerForwarded: "for=client.invalid"}},
+		"Invalid-X-Original-Forwarded-For": {header: map[string]string{headerXOriginalForwardedFor: "client.invalid"}},
+		"Invalid-ForwardedMissingFor":      {header: map[string]string{headerForwarded: "128.0.0.5"}},
+		"X-Forwarded-For": {
+			header: map[string]string{headerXForwardedFor: "123.0.0.1"},
+			ip:     "123.0.0.1"},
+		"X-Forwarded-For-First": {
+			header: map[string]string{headerXForwardedFor: "123.0.0.1, 127.0.0.1"},
+			ip:     "123.0.0.1"},
+		"X-Real-IP": {
+			header: map[string]string{headerXRealIP: "123.0.0.1:6060"},
+			ip:     "123.0.0.1"},
+		"X-Real-IP-Fallback": {
+			header: map[string]string{headerXRealIP: "invalid", headerXForwardedFor: "182.0.0.9"},
+			ip:     "182.0.0.9"},
+		"Forwarded": {
+			header: map[string]string{headerForwarded: "for=[2001:db8:cafe::17]:4711"},
+			ip:     "2001:db8:cafe::17"},
+		"Forwarded-Fallback": {
+			header: map[string]string{headerForwarded: "invalid", headerXForwardedFor: "182.0.0.9"},
+			ip:     "182.0.0.9"},
+		"Forwarded-Fallback2": {
+			header: map[string]string{headerForwarded: "invalid", headerXRealIP: "182.0.0.9"},
+			ip:     "182.0.0.9"},
+		"X-Original-Forwarded-For": {
+			header: map[string]string{headerXOriginalForwardedFor: "192.169.56.101"},
+			ip:     "192.169.56.101"},
+	} {
+		t.Run("invalid"+name, func(t *testing.T) {
+			header := make(http.Header)
+			for k, v := range tc.header {
+				header.Set(k, v)
+			}
+			ip := utility.ExtractIPFromHeaderRum(header)
+			if tc.ip == "" {
+				assert.Nil(t, ip)
+			} else {
+				require.NotNil(t, ip)
+				assert.Equal(t, tc.ip, ip.String())
+			}
+		})
+	}
+}
+
 func TestParseIP(t *testing.T) {
 	for name, tc := range map[string]struct {
 		inp  string

utility/remoteaddr.go

@@ -23,6 +23,13 @@ import (
 	"strings"
 )
 
+var parseRumHeadersInOrder = []func(http.Header) string{
+	parseXOriginalForwardedFor,
+	parseForwarded,
+	parseXRealIP,
+	parseXForwardedFor,
+}
+
 var parseHeadersInOrder = []func(http.Header) string{
 	parseForwarded,
 	parseXRealIP,
@@ -32,13 +39,13 @@ var parseHeadersInOrder = []func(http.Header) string{
 // RemoteAddr returns the remote address for the HTTP request.
 //
 // In order:
-//  - if the Forwarded header is set, then the first item in the
-//    list's "for" field is used, if it exists. The "for" value
-//    is returned even if it is an obfuscated identifier.
-//  - if the X-Real-Ip header is set, then its value is returned.
-//  - if the X-Forwarded-For header is set, then the first value
-//    in the comma-separated list is returned.
-//  - otherwise, the host portion of req.RemoteAddr is returned.
+//   - if the Forwarded header is set, then the first item in the
+//     list's "for" field is used, if it exists. The "for" value
+//     is returned even if it is an obfuscated identifier.
+//   - if the X-Real-Ip header is set, then its value is returned.
+//   - otherwise, the host portion of req.RemoteAddr is returned.
+//   - if the X-Forwarded-For header is set, then the first value
+//     in the comma-separated list is returned.
 //
 // Because the client can control the headers, they can control
 // the result of this function. The result should therefore not
@@ -91,3 +98,13 @@ func parseXForwardedFor(header http.Header) string {
 	}
 	return ""
 }
+
+func parseXOriginalForwardedFor(header http.Header) string {
+	if xoff := header.Get("X-Original-Forwarded-For"); xoff != "" {
+		if sep := strings.IndexRune(xoff, ','); sep > 0 {
+			xoff = xoff[:sep]
+		}
+		return strings.TrimSpace(xoff)
+	}
+	return ""
+}

version.yaml

@@ -1 +1 @@
-AppVersion: 0.1.24
+AppVersion: 0.1.25