CSRF middleware: Add flag to skip login cookie check (#66806)

* CSRF middleware: add flag to skip login cookie check * Update docs/sources/setup-grafana/configure-grafana/_index.md Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com> --------- Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
2023-04-24 10:11:08 -03:00
parent 5f16cd5124
commit d4715a6f04
6 changed files with 141 additions and 35 deletions
@@ -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 {
@@ -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)
+	}
+}