From a1579283a642a5fb43872bf505fff1da3f992017 Mon Sep 17 00:00:00 2001
From: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Date: Tue, 14 Jan 2020 17:41:54 +0100
Subject: [PATCH] Add disabled option for cookie samesite attribute (#21472)

Breaking change: If disabled the cookie samesite cookie attribute
will not be set, but if none the attribute will be set and is a
breaking change compared to before where none did not render the
attribute. This was due to a known issue in Safari.

Co-Authored-By: Arve Knudsen <arve.knudsen@gmail.com>
Co-Authored-By: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>

Fixes #19847
---
 CHANGELOG.md                               |  1 +
 conf/defaults.ini                          |  2 +-
 conf/sample.ini                            |  2 +-
 docs/sources/installation/configuration.md |  8 +++---
 docs/sources/installation/upgrading.md     | 11 ++++++++
 docs/sources/reference/share_panel.md      |  2 +-
 pkg/api/login.go                           |  7 +++--
 pkg/api/login_test.go                      |  6 ++--
 pkg/middleware/cookie.go                   | 18 ++++++------
 pkg/middleware/middleware_test.go          | 24 ++++++++++++----
 pkg/setting/setting.go                     | 32 ++++++++++++++--------
 11 files changed, 75 insertions(+), 38 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d54703eb3f1..02b4d7280a7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 ## Breaking changes
 * **PagerDuty**: Change `payload.custom_details` field in PagerDuty notification to be a JSON object instead of a string.
+* **Security**: The `[security]` setting `cookie_samesite` configured to `none` now renders cookies with `SameSite=None` attribute compared to before where no `SameSite` attribute was added to cookies. To get the old behavior, use value `disabled` instead of `none`. Refer to [Upgrade Grafana](https://grafana.com/docs/grafana/latest/installation/upgrading/#upgrading-to-v6-6) for more information.
 
 # 6.5.2 (2019-12-11)
 
diff --git a/conf/defaults.ini b/conf/defaults.ini
index 2a7f9ab558e..07b0070b187 100644
--- a/conf/defaults.ini
+++ b/conf/defaults.ini
@@ -179,7 +179,7 @@ disable_brute_force_login_protection = false
 # set to true if you host Grafana behind HTTPS. default is false.
 cookie_secure = false
 
-# set cookie SameSite attribute. defaults to `lax`. can be set to "lax", "strict" and "none"
+# set cookie SameSite attribute. defaults to `lax`. can be set to "lax", "strict", "none" and "disabled"
 cookie_samesite = lax
 
 # set to true if you want to allow browsers to render Grafana in a <frame>, <iframe>, <embed> or <object>. default is false.
diff --git a/conf/sample.ini b/conf/sample.ini
index 25f3d999f04..b83039d2552 100644
--- a/conf/sample.ini
+++ b/conf/sample.ini
@@ -180,7 +180,7 @@
 # set to true if you host Grafana behind HTTPS. default is false.
 ;cookie_secure = false
 
-# set cookie SameSite attribute. defaults to `lax`. can be set to "lax", "strict" and "none"
+# set cookie SameSite attribute. defaults to `lax`. can be set to "lax", "strict", "none" and "disabled"
 ;cookie_samesite = lax
 
 # set to true if you want to allow browsers to render Grafana in a <frame>, <iframe>, <embed> or <object>. default is false.
diff --git a/docs/sources/installation/configuration.md b/docs/sources/installation/configuration.md
index 830e5dae389..dc522a07ee0 100755
--- a/docs/sources/installation/configuration.md
+++ b/docs/sources/installation/configuration.md
@@ -24,14 +24,14 @@ Do not change `defaults.ini`! Grafana defaults are stored in this file. Dependin
 - Custom configuration from `$WORKING_DIR/conf/custom.ini`
 - The custom configuration file path can be overridden using the `--config` parameter
 
-**Linux** 
+**Linux**
 
 If you installed Grafana using the `deb` or `rpm` packages, then your configuration file is located at `/etc/grafana/grafana.ini` and a separate `custom.ini` is not used. This path is specified in the Grafana init.d script using `--config` file parameter.
 
 **Windows**
 `sample.ini` is in the same directory as `defaults.ini` and contains all the settings commented out. Copy `sample.ini` and name it `custom.ini`.
 
-**macOS** 
+**macOS**
 By default, the configuration file is located at `/usr/local/etc/grafana/grafana.ini`.
 
 ## Comments in .ini Files
@@ -361,7 +361,7 @@ Set to `true` if you host Grafana behind HTTPS. Default is `false`.
 
 ### cookie_samesite
 
-Sets the `SameSite` cookie attribute and prevents the browser from sending this cookie along with cross-site requests. The main goal is mitigate the risk of cross-origin information leakage. It also provides some protection against cross-site request forgery attacks (CSRF),  [read more here](https://www.owasp.org/index.php/SameSite). Valid values are `lax`, `strict` and `none`. Default is `lax`.
+Sets the `SameSite` cookie attribute and prevents the browser from sending this cookie along with cross-site requests. The main goal is to mitigate the risk of cross-origin information leakage. This setting also provides some protection against cross-site request forgery attacks (CSRF),  [read more about SameSite here](https://www.owasp.org/index.php/SameSite). Valid values are `lax`, `strict`, `none`, and `disabled`. Default is `lax`. Using value `disabled` does not add any `SameSite` attribute to cookies.
 
 ### allow_embedding
 
@@ -611,7 +611,7 @@ You can choose between (s3, webdav, gcs, azure_blob, local). If left empty Grafa
 ## [external_image_storage.s3]
 
 ### endpoint
-Optional endpoint URL (hostname or fully qualified URI) to override the default generated S3 endpoint. If you want to 
+Optional endpoint URL (hostname or fully qualified URI) to override the default generated S3 endpoint. If you want to
 keep the default, just leave this empty. You must still provide a `region` value if you specify an endpoint.
 
 ## path_style_access
diff --git a/docs/sources/installation/upgrading.md b/docs/sources/installation/upgrading.md
index 720d71406cd..d729728d910 100755
--- a/docs/sources/installation/upgrading.md
+++ b/docs/sources/installation/upgrading.md
@@ -226,3 +226,14 @@ The handling of multi-valued template variables in dimension values has been cha
 ## Upgrading to v6.6
 
 The Generic OAuth setting `send_client_credentials_via_post`, used for supporting non-compliant providers, has been removed. From now on, Grafana will automatically detect if credentials should be sent as part of the URL or request body for a specific provider. The result will be remembered and used for additional OAuth requests for that provider.
+
+### Important changes regarding SameSite cookie attribute
+
+Chrome 80 treats cookies as `SameSite=Lax` by default if no `SameSite` attribute is specified, see https://www.chromestatus.com/feature/5088147346030592.
+
+Due to this change in Chrome, the `[security]` setting `cookie_samesite` configured to `none` now renders cookies with `SameSite=None` attribute compared to before where no `SameSite` attribute was added to cookies. To get the old behavior, use value `disabled` instead of `none`, see [cookie_samesite in Configuration]({{< relref "configuration/#cookie-samesite" >}}) for more information.
+
+**Note:** There is currently a bug affecting Mac OSX and iOS that causes `SameSite=None` cookies to be treated as `SameSite=Strict` and therefore not sent with cross-site requests. (See https://bugs.webkit.org/show_bug.cgi?id=198181.) Until this is fixed, `SameSite=None` might not work properly on Safari.
+
+This version of Chrome also rejects insecure `SameSite=None` cookies. See https://www.chromestatus.com/feature/5633521622188032 for more information. Make sure that you
+change the `[security]` setting `cookie_secure` to `true` and use HTTPS when `cookie_samesite` is configured to `none`, otherwise authentication in Grafana won't work properly.
diff --git a/docs/sources/reference/share_panel.md b/docs/sources/reference/share_panel.md
index 5f9c0a998aa..a54b373a334 100644
--- a/docs/sources/reference/share_panel.md
+++ b/docs/sources/reference/share_panel.md
@@ -32,7 +32,7 @@ https://play.grafana.org/d/000000012/grafana-play-home?orgId=1&from=156871968017
 
 You can embed a panel using an iframe on another web site. This tab will show you the html that you need to use.
 
-> *Notice* This sharing require either anonymous access or setting [cookie_samesite]({{< relref "../installation/configuration.md#cookie-samesite" >}}) to none
+> **Note:** This sharing requires [allow_embedding]({{< relref "../installation/configuration.md#allow-embedding" >}}) enabled and anonymous access, or proper configuration of the [cookie_samesite]({{< relref "../installation/configuration.md#cookie-samesite" >}}) setting.
 
 Example:
 
diff --git a/pkg/api/login.go b/pkg/api/login.go
index 48af35c2ef4..e227b9b4940 100644
--- a/pkg/api/login.go
+++ b/pkg/api/login.go
@@ -43,9 +43,10 @@ func (hs *HTTPServer) validateRedirectTo(redirectTo string) error {
 
 func (hs *HTTPServer) cookieOptionsFromCfg() middleware.CookieOptions {
 	return middleware.CookieOptions{
-		Path:     hs.Cfg.AppSubUrl + "/",
-		Secure:   hs.Cfg.CookieSecure,
-		SameSite: hs.Cfg.CookieSameSite,
+		Path:             hs.Cfg.AppSubUrl + "/",
+		Secure:           hs.Cfg.CookieSecure,
+		SameSiteDisabled: hs.Cfg.CookieSameSiteDisabled,
+		SameSiteMode:     hs.Cfg.CookieSameSiteMode,
 	}
 }
 
diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go
index 684f3ddccce..05aa4a0345d 100644
--- a/pkg/api/login_test.go
+++ b/pkg/api/login_test.go
@@ -113,7 +113,7 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) {
 		HttpOnly: true,
 		Path:     setting.AppSubUrl + "/",
 		Secure:   hs.Cfg.CookieSecure,
-		SameSite: hs.Cfg.CookieSameSite,
+		SameSite: hs.Cfg.CookieSameSiteMode,
 	}
 	sc.m.Get(sc.url, sc.defaultHandler)
 	sc.fakeReqNoAssertionsWithCookie("GET", sc.url, cookie).exec()
@@ -204,7 +204,7 @@ func TestLoginViewRedirect(t *testing.T) {
 				HttpOnly: true,
 				Path:     hs.Cfg.AppSubUrl + "/",
 				Secure:   hs.Cfg.CookieSecure,
-				SameSite: hs.Cfg.CookieSameSite,
+				SameSite: hs.Cfg.CookieSameSiteMode,
 			}
 			sc.m.Get(sc.url, sc.defaultHandler)
 			sc.fakeReqNoAssertionsWithCookie("GET", sc.url, cookie).exec()
@@ -312,7 +312,7 @@ func TestLoginPostRedirect(t *testing.T) {
 				HttpOnly: true,
 				Path:     hs.Cfg.AppSubUrl + "/",
 				Secure:   hs.Cfg.CookieSecure,
-				SameSite: hs.Cfg.CookieSameSite,
+				SameSite: hs.Cfg.CookieSameSiteMode,
 			}
 			sc.m.Post(sc.url, sc.defaultHandler)
 			sc.fakeReqNoAssertionsWithCookie("POST", sc.url, cookie).exec()
diff --git a/pkg/middleware/cookie.go b/pkg/middleware/cookie.go
index 175aca60ce1..895b98f4e8f 100644
--- a/pkg/middleware/cookie.go
+++ b/pkg/middleware/cookie.go
@@ -7,16 +7,18 @@ import (
 )
 
 type CookieOptions struct {
-	Path     string
-	Secure   bool
-	SameSite http.SameSite
+	Path             string
+	Secure           bool
+	SameSiteDisabled bool
+	SameSiteMode     http.SameSite
 }
 
 func newCookieOptions() CookieOptions {
 	return CookieOptions{
-		Path:     setting.AppSubUrl + "/",
-		Secure:   setting.CookieSecure,
-		SameSite: setting.CookieSameSite,
+		Path:             setting.AppSubUrl + "/",
+		Secure:           setting.CookieSecure,
+		SameSiteDisabled: setting.CookieSameSiteDisabled,
+		SameSiteMode:     setting.CookieSameSiteMode,
 	}
 }
 
@@ -36,8 +38,8 @@ func WriteCookie(w http.ResponseWriter, name string, value string, maxAge int, g
 		Path:     options.Path,
 		Secure:   options.Secure,
 	}
-	if options.SameSite != http.SameSiteDefaultMode {
-		cookie.SameSite = options.SameSite
+	if !options.SameSiteDisabled {
+		cookie.SameSite = options.SameSiteMode
 	}
 	http.SetCookie(w, &cookie)
 }
diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go
index 897ddfa36d8..4045b2c6741 100644
--- a/pkg/middleware/middleware_test.go
+++ b/pkg/middleware/middleware_test.go
@@ -256,12 +256,12 @@ func TestMiddlewareContext(t *testing.T) {
 			maxAge := (maxAgeHours + time.Hour).Seconds()
 
 			sameSitePolicies := []http.SameSite{
-				http.SameSiteDefaultMode,
+				http.SameSiteNoneMode,
 				http.SameSiteLaxMode,
 				http.SameSiteStrictMode,
 			}
 			for _, sameSitePolicy := range sameSitePolicies {
-				setting.CookieSameSite = sameSitePolicy
+				setting.CookieSameSiteMode = sameSitePolicy
 				expectedCookie := &http.Cookie{
 					Name:     setting.LoginCookieName,
 					Value:    "rotated",
@@ -269,9 +269,7 @@ func TestMiddlewareContext(t *testing.T) {
 					HttpOnly: true,
 					MaxAge:   int(maxAge),
 					Secure:   setting.CookieSecure,
-				}
-				if sameSitePolicy != http.SameSiteDefaultMode {
-					expectedCookie.SameSite = sameSitePolicy
+					SameSite: sameSitePolicy,
 				}
 
 				sc.fakeReq("GET", "/").exec()
@@ -287,6 +285,22 @@ func TestMiddlewareContext(t *testing.T) {
 					So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, expectedCookie.String())
 				})
 			}
+
+			Convey("Should not set cookie with SameSite attribute when setting.CookieSameSiteDisabled is true", func() {
+				setting.CookieSameSiteDisabled = true
+				setting.CookieSameSiteMode = http.SameSiteLaxMode
+				expectedCookie := &http.Cookie{
+					Name:     setting.LoginCookieName,
+					Value:    "rotated",
+					Path:     setting.AppSubUrl + "/",
+					HttpOnly: true,
+					MaxAge:   int(maxAge),
+					Secure:   setting.CookieSecure,
+				}
+
+				sc.fakeReq("GET", "/").exec()
+				So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, expectedCookie.String())
+			})
 		})
 
 		middlewareScenario(t, "Invalid/expired auth token in cookie", func(sc *scenarioContext) {
diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go
index a7269d741a7..370bdb060cc 100644
--- a/pkg/setting/setting.go
+++ b/pkg/setting/setting.go
@@ -101,7 +101,8 @@ var (
 	DataProxyWhiteList                map[string]bool
 	DisableBruteForceLoginProtection  bool
 	CookieSecure                      bool
-	CookieSameSite                    http.SameSite
+	CookieSameSiteDisabled            bool
+	CookieSameSiteMode                http.SameSite
 	AllowEmbedding                    bool
 	XSSProtectionHeader               bool
 	ContentTypeProtectionHeader       bool
@@ -248,7 +249,8 @@ type Cfg struct {
 	DisableInitAdminCreation         bool
 	DisableBruteForceLoginProtection bool
 	CookieSecure                     bool
-	CookieSameSite                   http.SameSite
+	CookieSameSiteDisabled           bool
+	CookieSameSiteMode               http.SameSite
 
 	TempDataLifetime                 time.Duration
 	MetricsEndpointEnabled           bool
@@ -719,18 +721,24 @@ func (cfg *Cfg) Load(args *CommandLineArgs) error {
 	if err != nil {
 		return err
 	}
-	validSameSiteValues := map[string]http.SameSite{
-		"lax":    http.SameSiteLaxMode,
-		"strict": http.SameSiteStrictMode,
-		"none":   http.SameSiteDefaultMode,
-	}
 
-	if samesite, ok := validSameSiteValues[samesiteString]; ok {
-		CookieSameSite = samesite
-		cfg.CookieSameSite = CookieSameSite
+	if samesiteString == "disabled" {
+		CookieSameSiteDisabled = true
+		cfg.CookieSameSiteDisabled = CookieSameSiteDisabled
 	} else {
-		CookieSameSite = http.SameSiteLaxMode
-		cfg.CookieSameSite = CookieSameSite
+		validSameSiteValues := map[string]http.SameSite{
+			"lax":    http.SameSiteLaxMode,
+			"strict": http.SameSiteStrictMode,
+			"none":   http.SameSiteNoneMode,
+		}
+
+		if samesite, ok := validSameSiteValues[samesiteString]; ok {
+			CookieSameSiteMode = samesite
+			cfg.CookieSameSiteMode = CookieSameSiteMode
+		} else {
+			CookieSameSiteMode = http.SameSiteLaxMode
+			cfg.CookieSameSiteMode = CookieSameSiteMode
+		}
 	}
 
 	AllowEmbedding = security.Key("allow_embedding").MustBool(false)