[MM-15639] Add config setting to explicitly define which IP headers are trusted (#10907)

* Add config setting to explicitly define which IP headers are trusted

* fix variable shadowing

* Optimize code flow; Add Ratelimit test for header set

* Extend Ratelimit tests

* Add additional unit tests

* Structured logging

app/app.go

@@ -4,7 +4,6 @@
 package app
 
 import (
-	"fmt"
 	"html/template"
 	"net/http"
 	"strconv"
@@ -134,7 +133,8 @@ func (a *App) HTMLTemplates() *template.Template {
 }
 
 func (a *App) Handle404(w http.ResponseWriter, r *http.Request) {
-	mlog.Debug(fmt.Sprintf("%v: code=404 ip=%v", r.URL.Path, utils.GetIpAddress(r)))
+	ipAddress := utils.GetIpAddress(r, a.Config().ServiceSettings.TrustedProxyIPHeader)
+	mlog.Debug("not found handler triggered", mlog.String("path", r.URL.Path), mlog.Int("code", 404), mlog.String("ip", ipAddress))
 
 	if *a.Config().ServiceSettings.WebserverMode == "disabled" {
 		http.NotFound(w, r)

app/plugin_requests.go

@@ -74,7 +74,7 @@ func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler
 	token := ""
 	context := &plugin.Context{
 		RequestId:      model.NewId(),
-		IpAddress:      utils.GetIpAddress(r),
+		IpAddress:      utils.GetIpAddress(r, a.Config().ServiceSettings.TrustedProxyIPHeader),
 		AcceptLanguage: r.Header.Get("Accept-Language"),
 		UserAgent:      r.UserAgent(),
 	}

app/ratelimit.go

@@ -23,9 +23,10 @@ type RateLimiter struct {
 	useAuth              bool
 	useIP                bool
 	header               string
+	trustedProxyIPHeader []string
 }
 
-func NewRateLimiter(settings *model.RateLimitSettings) (*RateLimiter, error) {
+func NewRateLimiter(settings *model.RateLimitSettings, trustedProxyIPHeader []string) (*RateLimiter, error) {
 	store, err := memstore.New(*settings.MemoryStoreSize)
 	if err != nil {
 		return nil, errors.Wrap(err, utils.T("api.server.start_server.rate_limiting_memory_store"))
@@ -46,6 +47,7 @@ func NewRateLimiter(settings *model.RateLimitSettings) (*RateLimiter, error) {
 		useAuth:              *settings.VaryByUser,
 		useIP:                *settings.VaryByRemoteAddr,
 		header:               settings.VaryByHeader,
+		trustedProxyIPHeader: trustedProxyIPHeader,
 	}, nil
 }
 
@@ -57,10 +59,10 @@ func (rl *RateLimiter) GenerateKey(r *http.Request) string {
 		if tokenLocation != TokenLocationNotFound {
 			key += token
 		} else if rl.useIP { // If we don't find an authentication token and IP based is enabled, fall back to IP
-			key += utils.GetIpAddress(r)
+			key += utils.GetIpAddress(r, rl.trustedProxyIPHeader)
 		}
 	} else if rl.useIP { // Only if Auth based is not enabed do we use a plain IP based
-		key += utils.GetIpAddress(r)
+		key += utils.GetIpAddress(r, rl.trustedProxyIPHeader)
 	}
 
 	// Note that most of the time the user won't have to set this because the utils.GetIpAddress above tries the

app/ratelimit_test.go

@@ -27,7 +27,11 @@ func genRateLimitSettings(useAuth, useIP bool, header string) *model.RateLimitSe
 
 func TestNewRateLimiterSuccess(t *testing.T) {
 	settings := genRateLimitSettings(false, false, "")
-	rateLimiter, err := NewRateLimiter(settings)
+	rateLimiter, err := NewRateLimiter(settings, nil)
+	require.NotNil(t, rateLimiter)
+	require.NoError(t, err)
+
+	rateLimiter, err = NewRateLimiter(settings, []string{"X-Forwarded-For"})
 	require.NotNil(t, rateLimiter)
 	require.NoError(t, err)
 }
@@ -35,7 +39,11 @@ func TestNewRateLimiterSuccess(t *testing.T) {
 func TestNewRateLimiterFailure(t *testing.T) {
 	invalidSettings := genRateLimitSettings(false, false, "")
 	invalidSettings.MaxBurst = model.NewInt(-100)
-	rateLimiter, err := NewRateLimiter(invalidSettings)
+	rateLimiter, err := NewRateLimiter(invalidSettings, nil)
+	require.Nil(t, rateLimiter)
+	require.Error(t, err)
+
+	rateLimiter, err = NewRateLimiter(invalidSettings, []string{"X-Forwarded-For", "X-Real-Ip"})
 	require.Nil(t, rateLimiter)
 	require.Error(t, err)
 }
@@ -73,10 +81,25 @@ func TestGenerateKey(t *testing.T) {
 			req.Header.Set(tc.header, tc.headerResult)
 		}
 
-		rateLimiter, _ := NewRateLimiter(genRateLimitSettings(tc.useAuth, tc.useIP, tc.header))
+		rateLimiter, _ := NewRateLimiter(genRateLimitSettings(tc.useAuth, tc.useIP, tc.header), nil)
 
 		key := rateLimiter.GenerateKey(req)
 
 		require.Equal(t, tc.expectedKey, key, "Wrong key on test "+strconv.Itoa(testnum))
 	}
 }
+
+func TestGenerateKey_TrustedHeader(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.10.10.5:80"
+	req.Header.Set("X-Forwarded-For", "10.6.3.1, 10.5.1.2")
+
+
+	rateLimiter, _ := NewRateLimiter(genRateLimitSettings(true, true, ""), []string{"X-Forwarded-For"})
+	key := rateLimiter.GenerateKey(req)
+	require.Equal(t, "10.6.3.1", key, "Wrong key on test with allowed trusted proxy header")
+
+	rateLimiter, _ = NewRateLimiter(genRateLimitSettings(true, true, ""), nil)
+	key = rateLimiter.GenerateKey(req)
+	require.Equal(t, "10.10.10.5", key, "Wrong key on test without allowed trusted proxy header")
+}

app/server.go

@@ -440,7 +440,7 @@ func (s *Server) Start() error {
 	if *s.Config().RateLimitSettings.Enable {
 		mlog.Info("RateLimiter is enabled")
 
-		rateLimiter, err := NewRateLimiter(&s.Config().RateLimitSettings)
+		rateLimiter, err := NewRateLimiter(&s.Config().RateLimitSettings, s.Config().ServiceSettings.TrustedProxyIPHeader)
 		if err != nil {
 			return err
 		}

config/default.json

@@ -14,6 +14,7 @@
         "UseLetsEncrypt": false,
         "LetsEncryptCertificateCacheFile": "./config/letsencrypt.cache",
         "Forward80To443": false,
+        "TrustedProxyIPHeader": [],
         "ReadTimeout": 300,
         "WriteTimeout": 300,
         "MaximumLoginAttempts": 10,

model/config.go

@@ -226,6 +226,7 @@ type ServiceSettings struct {
 	UseLetsEncrypt                                    *bool    `restricted:"true"`
 	LetsEncryptCertificateCacheFile                   *string  `restricted:"true"`
 	Forward80To443                                    *bool    `restricted:"true"`
+	TrustedProxyIPHeader                              []string `restricted:"true"`
 	ReadTimeout                                       *int     `restricted:"true"`
 	WriteTimeout                                      *int     `restricted:"true"`
 	MaximumLoginAttempts                              *int     `restricted:"true"`
@@ -434,6 +435,10 @@ func (s *ServiceSettings) SetDefaults() {
 		s.Forward80To443 = NewBool(false)
 	}
 
+	if s.TrustedProxyIPHeader == nil {
+		s.TrustedProxyIPHeader = []string{HEADER_FORWARDED, HEADER_REAL_IP}
+	}
+
 	if s.TimeBetweenUserTypingUpdatesMilliseconds == nil {
 		s.TimeBetweenUserTypingUpdatesMilliseconds = NewInt64(5000)
 	}

utils/utils.go

@@ -8,8 +8,6 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
-
-	"github.com/mattermost/mattermost-server/model"
 )
 
 func StringInSlice(a string, slice []string) bool {
@@ -68,19 +66,21 @@ func StringSliceDiff(a, b []string) []string {
 	return result
 }
 
-func GetIpAddress(r *http.Request) string {
+func GetIpAddress(r *http.Request, trustedProxyIPHeader []string) string {
 	address := ""
 
-	header := r.Header.Get(model.HEADER_FORWARDED)
+	for _, proxyHeader := range trustedProxyIPHeader {
-	if len(header) > 0 {
+		header := r.Header.Get(proxyHeader)
-		addresses := strings.Fields(header)
+		if len(header) > 0 {
-		if len(addresses) > 0 {
+			addresses := strings.Fields(header)
-			address = strings.TrimRight(addresses[0], ",")
+			if len(addresses) > 0 {
+				address = strings.TrimRight(addresses[0], ",")
+			}
 		}
-	}
 
-	if len(address) == 0 {
+		if len(address) > 0 {
-		address = r.Header.Get(model.HEADER_REAL_IP)
+			return address
+		}
 	}
 
 	if len(address) == 0 {

utils/utils_test.go

@@ -66,7 +66,7 @@ func TestGetIpAddress(t *testing.T) {
 		RemoteAddr: "10.2.0.1:12345",
 	}
 
-	assert.Equal(t, "10.0.0.1", GetIpAddress(&httpRequest1))
+	assert.Equal(t, "10.0.0.1", GetIpAddress(&httpRequest1, []string{"X-Forwarded-For"}))
 
 	// Test with multiple IPs in the X-Forwarded-For
 	httpRequest2 := http.Request{
@@ -77,7 +77,7 @@ func TestGetIpAddress(t *testing.T) {
 		RemoteAddr: "10.2.0.1:12345",
 	}
 
-	assert.Equal(t, "10.0.0.1", GetIpAddress(&httpRequest2))
+	assert.Equal(t, "10.0.0.1", GetIpAddress(&httpRequest2, []string{"X-Forwarded-For"}))
 
 	// Test with an empty X-Forwarded-For
 	httpRequest3 := http.Request{
@@ -88,7 +88,7 @@ func TestGetIpAddress(t *testing.T) {
 		RemoteAddr: "10.2.0.1:12345",
 	}
 
-	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest3))
+	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest3, []string{"X-Forwarded-For", "X-Real-Ip"}))
 
 	// Test without an X-Fowarded-For
 	httpRequest4 := http.Request{
@@ -98,12 +98,65 @@ func TestGetIpAddress(t *testing.T) {
 		RemoteAddr: "10.2.0.1:12345",
 	}
 
-	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest4))
+	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest4, []string{"X-Forwarded-For", "X-Real-Ip"}))
 
 	// Test without any headers
 	httpRequest5 := http.Request{
 		RemoteAddr: "10.2.0.1:12345",
 	}
 
-	assert.Equal(t, "10.2.0.1", GetIpAddress(&httpRequest5))
+	assert.Equal(t, "10.2.0.1", GetIpAddress(&httpRequest5, []string{"X-Forwarded-For", "X-Real-Ip"}))
+
+	// Test with both headers, but both untrusted
+	httpRequest6 := http.Request{
+		Header: http.Header{
+			"X-Forwarded-For": []string{"10.3.0.1"},
+			"X-Real-Ip":       []string{"10.1.0.1"},
+		},
+		RemoteAddr: "10.2.0.1:12345",
+	}
+
+	assert.Equal(t, "10.2.0.1", GetIpAddress(&httpRequest6, nil))
+
+	// Test with both headers, but only X-Real-Ip trusted
+	httpRequest7 := http.Request{
+		Header: http.Header{
+			"X-Forwarded-For": []string{"10.3.0.1"},
+			"X-Real-Ip":       []string{"10.1.0.1"},
+		},
+		RemoteAddr: "10.2.0.1:12345",
+	}
+
+	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest7, []string{"X-Real-Ip"}))
+
+	// Test with X-Forwarded-For, comma separated, untrusted
+	httpRequest8 := http.Request{
+		Header: http.Header{
+			"X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"},
+		},
+		RemoteAddr: "10.2.0.1:12345",
+	}
+
+	assert.Equal(t, "10.2.0.1", GetIpAddress(&httpRequest8, nil))
+
+	// Test with X-Forwarded-For, comma separated, untrusted
+	httpRequest9 := http.Request{
+		Header: http.Header{
+			"X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"},
+		},
+		RemoteAddr: "10.2.0.1:12345",
+	}
+
+	assert.Equal(t, "10.3.0.1", GetIpAddress(&httpRequest9, []string{"X-Forwarded-For"}))
+
+	// Test with both headers, both allowed, first one in trusted used
+	httpRequest10 := http.Request{
+		Header: http.Header{
+			"X-Forwarded-For": []string{"10.3.0.1"},
+			"X-Real-Ip":       []string{"10.1.0.1"},
+		},
+		RemoteAddr: "10.2.0.1:12345",
+	}
+
+	assert.Equal(t, "10.1.0.1", GetIpAddress(&httpRequest10, []string{"X-Real-Ip", "X-Forwarded-For"}))
 }

web/handlers.go

@@ -63,7 +63,7 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	)
 	c.App.T, _ = utils.GetTranslationsAndLocale(w, r)
 	c.App.RequestId = model.NewId()
-	c.App.IpAddress = utils.GetIpAddress(r)
+	c.App.IpAddress = utils.GetIpAddress(r, c.App.Config().ServiceSettings.TrustedProxyIPHeader)
 	c.App.UserAgent = r.UserAgent()
 	c.App.AcceptLanguage = r.Header.Get("Accept-Language")
 	c.Params = ParamsFromRequest(r)

web/web.go

@@ -4,7 +4,6 @@
 package web
 
 import (
-	"fmt"
 	"net/http"
 	"path"
 	"strings"
@@ -61,8 +60,8 @@ func CheckClientCompatability(agentString string) bool {
 
 func Handle404(config configservice.ConfigService, w http.ResponseWriter, r *http.Request) {
 	err := model.NewAppError("Handle404", "api.context.404.app_error", nil, "", http.StatusNotFound)
-
+	ipAddress := utils.GetIpAddress(r, config.Config().ServiceSettings.TrustedProxyIPHeader)
-	mlog.Debug(fmt.Sprintf("%v: code=404 ip=%v", r.URL.Path, utils.GetIpAddress(r)))
+	mlog.Debug("not found handler triggered", mlog.String("path", r.URL.Path), mlog.Int("code", 404), mlog.String("ip", ipAddress))
 
 	if IsApiCall(config, r) {
 		w.WriteHeader(err.StatusCode)