diff --git a/conf/defaults.ini b/conf/defaults.ini index 14eda0a60a3..086c84d7348 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -369,6 +369,9 @@ content_security_policy_report_only_template = """script-src 'self' 'unsafe-eval # Controls if old angular plugins are supported or not. This will be disabled by default in future release angular_support_enabled = true +# The CSRF check will be executed even if the request has no login cookie. +csrf_always_check = false + [security.encryption] # Defines the time-to-live (TTL) for decrypted data encryption keys stored in memory (cache). # Please note that small values may cause performance issues due to a high frequency decryption operations. diff --git a/conf/sample.ini b/conf/sample.ini index 69b6dccfda1..d1d788a982b 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -375,6 +375,9 @@ # List of allowed headers to be set by the user, separated by spaces. Suggested to use for if authentication lives behind reverse proxies. ;csrf_additional_headers = +# The CSRF check will be executed even if the request has no login cookie. +;csrf_always_check = false + [security.encryption] # Defines the time-to-live (TTL) for decrypted data encryption keys stored in memory (cache). # Please note that small values may cause performance issues due to a high frequency decryption operations. diff --git a/docs/sources/setup-grafana/configure-grafana/_index.md b/docs/sources/setup-grafana/configure-grafana/_index.md index 1d1e3f96fe1..6072beb2ca1 100644 --- a/docs/sources/setup-grafana/configure-grafana/_index.md +++ b/docs/sources/setup-grafana/configure-grafana/_index.md @@ -679,6 +679,10 @@ List of additional allowed URLs to pass by the CSRF check. Suggested when authen List of allowed headers to be set by the user. Suggested to use for if authentication lives behind reverse proxies. +### csrf_always_check + +Set to `true` to execute the CSRF check even if the login cookie is not in a request (default `false`). + ## [snapshots] ### enabled diff --git a/pkg/middleware/csrf/csrf.go b/pkg/middleware/csrf/csrf.go index 85ca6069837..b03dee95d7d 100644 --- a/pkg/middleware/csrf/csrf.go +++ b/pkg/middleware/csrf/csrf.go @@ -24,9 +24,10 @@ type CSRF struct { trustedOrigins map[string]struct{} headers map[string]struct{} safeEndpoints map[string]struct{} + alwaysCheck bool } -func ProvideCSRFFilter(cfg *setting.Cfg) Service { +func ProvideCSRFFilter(cfg *setting.Cfg) *CSRF { c := &CSRF{ cfg: cfg, trustedOrigins: map[string]struct{}{}, @@ -36,6 +37,7 @@ func ProvideCSRFFilter(cfg *setting.Cfg) Service { additionalHeaders := cfg.SectionWithEnvOverrides("security").Key("csrf_additional_headers").Strings(" ") trustedOrigins := cfg.SectionWithEnvOverrides("security").Key("csrf_trusted_origins").Strings(" ") + c.alwaysCheck = cfg.SectionWithEnvOverrides("security").Key("csrf_always_check").MustBool(false) for _, header := range additionalHeaders { c.headers[header] = struct{}{} @@ -71,10 +73,14 @@ func (c *CSRF) check(r *http.Request) error { // (GET is excluded because it may have side effects in some APIs) safeMethods := []string{"HEAD", "OPTIONS", "TRACE"} - // If request has no login cookie - skip CSRF checks - if _, err := r.Cookie(c.cfg.LoginCookieName); errors.Is(err, http.ErrNoCookie) { - return nil + // If the CSRF checks can be skipped. + if !c.alwaysCheck { + // If request has no login cookie - skip CSRF checks + if _, err := r.Cookie(c.cfg.LoginCookieName); errors.Is(err, http.ErrNoCookie) { + return nil + } } + // Skip CSRF checks for "safe" methods for _, method := range safeMethods { if r.Method == method { diff --git a/pkg/middleware/csrf/csrf_test.go b/pkg/middleware/csrf/csrf_test.go index 470f20613ae..1214ff0e998 100644 --- a/pkg/middleware/csrf/csrf_test.go +++ b/pkg/middleware/csrf/csrf_test.go @@ -106,6 +106,7 @@ func TestCSRF_Check(t *testing.T) { tests := []struct { name string request *http.Request + getCfg func() *setting.Cfg addtHeader map[string]struct{} trustedOrigins map[string]struct{} safeEndpoints map[string]struct{} @@ -113,70 +114,116 @@ func TestCSRF_Check(t *testing.T) { expectedStatus int }{ { - name: "base case", - request: postRequest(t, "", nil), + name: "base case", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "", nil, true), expectedOK: true, }, { - name: "base with null origin header", - request: postRequest(t, "", map[string]string{"Origin": "null"}), + name: "base with null origin header", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "", map[string]string{"Origin": "null"}, true), expectedStatus: http.StatusForbidden, }, { - name: "grafana.org", - request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.org"}), + name: "grafana.org", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.org"}, true), expectedOK: true, }, { - name: "grafana.org with X-Forwarded-Host", - request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}), + name: "grafana.org with X-Forwarded-Host", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}, true), expectedStatus: http.StatusForbidden, }, { - name: "grafana.org with X-Forwarded-Host and header trusted", - request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}), + name: "grafana.org with X-Forwarded-Host and header trusted", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}, true), addtHeader: map[string]struct{}{"X-Forwarded-Host": {}}, expectedOK: true, }, { - name: "grafana.org from grafana.com", - request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}), + name: "grafana.org from grafana.com", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}, true), expectedStatus: http.StatusForbidden, }, { - name: "grafana.org from grafana.com explicit trust for grafana.com", - request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}), + name: "grafana.org from grafana.com explicit trust for grafana.com", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}, true), trustedOrigins: map[string]struct{}{"grafana.com": {}}, expectedOK: true, }, { - name: "grafana.org from grafana.com with X-Forwarded-Host and header trusted", - request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.com"}), + name: "grafana.org from grafana.com with X-Forwarded-Host and header trusted", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.com"}, true), addtHeader: map[string]struct{}{"X-Forwarded-Host": {}}, trustedOrigins: map[string]struct{}{"grafana.com": {}}, expectedOK: true, }, { - name: "safe endpoint", - request: postRequest(t, "example.org/foo/bar", map[string]string{"Origin": "null"}), + name: "safe endpoint", + getCfg: func() *setting.Cfg { + return setting.NewCfg() + }, + request: postRequest(t, "example.org/foo/bar", map[string]string{"Origin": "null"}, true), safeEndpoints: map[string]struct{}{"foo/bar": {}}, expectedOK: true, }, + { + name: "grafana.org with X-Forwarded-Host; will skip csrf check if login cookie is not present; without login cookie, should return nil because login cookie is not present", + getCfg: func() *setting.Cfg { + cfg := setting.NewCfg() + cfg.SectionWithEnvOverrides("security").Key("csrf_always_check").SetValue("false") + return cfg + }, + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}, false), + expectedOK: true, + }, + { + name: "grafana.org with X-Forwarded-Host; will perform csrf check even if login cookie is not present, should return error because host name does not match origin", + getCfg: func() *setting.Cfg { + cfg := setting.NewCfg() + cfg.SectionWithEnvOverrides("security").Key("csrf_always_check").SetValue("true") + return cfg + }, + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}, false), + expectedStatus: http.StatusForbidden, + }, } for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { - c := CSRF{ - cfg: setting.NewCfg(), - trustedOrigins: tc.trustedOrigins, - headers: tc.addtHeader, - safeEndpoints: tc.safeEndpoints, - } - c.cfg.LoginCookieName = "LoginCookie" + csrf := ProvideCSRFFilter(tc.getCfg()) + csrf.trustedOrigins = tc.trustedOrigins + csrf.headers = tc.addtHeader + csrf.safeEndpoints = tc.safeEndpoints + csrf.cfg.LoginCookieName = "LoginCookie" + + err := csrf.check(tc.request) - err := c.check(tc.request) if tc.expectedOK { require.NoError(t, err) } else { @@ -189,7 +236,7 @@ func TestCSRF_Check(t *testing.T) { } } -func postRequest(t testing.TB, hostname string, headers map[string]string) *http.Request { +func postRequest(t testing.TB, hostname string, headers map[string]string, withLoginCookie bool) *http.Request { t.Helper() urlParts := strings.SplitN(hostname, "/", 2) @@ -202,10 +249,12 @@ func postRequest(t testing.TB, hostname string, headers map[string]string) *http r.Host = urlParts[0] - r.AddCookie(&http.Cookie{ - Name: "LoginCookie", - Value: "this should not be important", - }) + if withLoginCookie { + r.AddCookie(&http.Cookie{ + Name: "LoginCookie", + Value: "this should not be important", + }) + } for k, v := range headers { r.Header.Set(k, v) @@ -240,3 +289,43 @@ func csrfScenario(t *testing.T, cookieName, method, origin, host string) *httpte handler.ServeHTTP(rr, req) return rr } + +func TestProvideCSRFFilter(t *testing.T) { + t.Parallel() + + tests := []struct { + getInput func() *setting.Cfg + expectedAlwaysCheck bool + }{ + { + getInput: func() *setting.Cfg { + return setting.NewCfg() + }, + // Should default to false when config value is not set. + expectedAlwaysCheck: false, + }, + { + getInput: func() *setting.Cfg { + cfg := setting.NewCfg() + cfg.SectionWithEnvOverrides("security").Key("csrf_always_check").SetValue("false") + return cfg + }, + // Should be false when config value is set to false. + expectedAlwaysCheck: false, + }, + { + getInput: func() *setting.Cfg { + cfg := setting.NewCfg() + cfg.SectionWithEnvOverrides("security").Key("csrf_always_check").SetValue("true") + return cfg + }, + // Should be true when config value is set to true. + expectedAlwaysCheck: true, + }, + } + + for _, tc := range tests { + csrf := ProvideCSRFFilter(tc.getInput()) + assert.Equal(t, tc.expectedAlwaysCheck, csrf.alwaysCheck) + } +} diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 8882bb443e6..4f3a7b625f8 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -312,6 +312,7 @@ var wireBasicSet = wire.NewSet( cuectx.GrafanaCUEContext, cuectx.GrafanaThemaRuntime, csrf.ProvideCSRFFilter, + wire.Bind(new(csrf.Service), new(*csrf.CSRF)), ossaccesscontrol.ProvideTeamPermissions, wire.Bind(new(accesscontrol.TeamPermissionsService), new(*ossaccesscontrol.TeamPermissionsService)), ossaccesscontrol.ProvideFolderPermissions,