[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
 		}