IntenseWebs/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2025-02-25 18:55:24 -06:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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
This commit is contained in:
Daniel Schalla
2019-05-24 20:22:13 +02:00
committed by GitHub
parent e8af4872c6
commit 2d97f01781
11 changed files with 113 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
app/app.go
View File

@@ -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)

2
app/plugin_requests.go
View File

@@ -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(),
}

8
app/ratelimit.go
View File

@@ -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

29
app/ratelimit_test.go
View File

@@ -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")
}

2
app/server.go
View File

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

1
config/default.json
View File

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

5
model/config.go
View File

@@ -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)
}

22
utils/utils.go
View File

@@ -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 {

63
utils/utils_test.go
View File

@@ -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"}))
}

2
web/handlers.go
View File

@@ -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)

5
web/web.go
View File

@@ -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)