diff --git a/app/app.go b/app/app.go index 4f884342ba..dd106ce4ba 100644 --- a/app/app.go +++ b/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) diff --git a/app/plugin_requests.go b/app/plugin_requests.go index 99425706cf..715daf087f 100644 --- a/app/plugin_requests.go +++ b/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(), } diff --git a/app/ratelimit.go b/app/ratelimit.go index d7b96dae34..0a7715f25b 100644 --- a/app/ratelimit.go +++ b/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 diff --git a/app/ratelimit_test.go b/app/ratelimit_test.go index fb157b2b00..1ae567e1bb 100644 --- a/app/ratelimit_test.go +++ b/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") +} diff --git a/app/server.go b/app/server.go index b7be402edb..5f0b11d4e9 100644 --- a/app/server.go +++ b/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 } diff --git a/config/default.json b/config/default.json index 762b8e64ff..b1a21dda21 100644 --- a/config/default.json +++ b/config/default.json @@ -14,6 +14,7 @@ "UseLetsEncrypt": false, "LetsEncryptCertificateCacheFile": "./config/letsencrypt.cache", "Forward80To443": false, + "TrustedProxyIPHeader": [], "ReadTimeout": 300, "WriteTimeout": 300, "MaximumLoginAttempts": 10, diff --git a/model/config.go b/model/config.go index 0e3672954a..5f80afe8c5 100644 --- a/model/config.go +++ b/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) } diff --git a/utils/utils.go b/utils/utils.go index b2d9f8dca7..3af6ebe240 100644 --- a/utils/utils.go +++ b/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) - if len(header) > 0 { - addresses := strings.Fields(header) - if len(addresses) > 0 { - address = strings.TrimRight(addresses[0], ",") + for _, proxyHeader := range trustedProxyIPHeader { + header := r.Header.Get(proxyHeader) + if len(header) > 0 { + addresses := strings.Fields(header) + if len(addresses) > 0 { + address = strings.TrimRight(addresses[0], ",") + } } - } - if len(address) == 0 { - address = r.Header.Get(model.HEADER_REAL_IP) + if len(address) > 0 { + return address + } } if len(address) == 0 { diff --git a/utils/utils_test.go b/utils/utils_test.go index 5ad76090ab..32c7b46acd 100644 --- a/utils/utils_test.go +++ b/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"})) } diff --git a/web/handlers.go b/web/handlers.go index 70ac08e8d2..71ad6af03e 100644 --- a/web/handlers.go +++ b/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) diff --git a/web/web.go b/web/web.go index 8be2d3eb0e..9e3e46ad19 100644 --- a/web/web.go +++ b/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) - - mlog.Debug(fmt.Sprintf("%v: code=404 ip=%v", r.URL.Path, utils.GetIpAddress(r))) + ipAddress := utils.GetIpAddress(r, config.Config().ServiceSettings.TrustedProxyIPHeader) + 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)