From 1b523390bb9ccf7cd5242759bb5c16fd27a5e273 Mon Sep 17 00:00:00 2001 From: Brian Kopp Date: Wed, 22 Jan 2020 13:19:16 -0700 Subject: [PATCH] Add SameSite=None support and conditionally omit SameSite=None for backwards compatibility --- docs/examples/affinity/cookie/README.md | 2 + .../affinity/cookie/ingress-samesite.yaml | 40 +++++++++++ .../nginx-configuration/annotations.md | 3 + .../annotations/sessionaffinity/main.go | 20 ++++++ internal/ingress/controller/controller.go | 2 + internal/ingress/types.go | 14 ++-- internal/ingress/types_equals.go | 6 ++ rootfs/etc/nginx/lua/balancer/sticky.lua | 15 +++++ .../nginx/lua/test/balancer/sticky_test.lua | 66 +++++++++++++++++++ .../nginx/lua/test/util/same_site_test.lua | 51 ++++++++++++++ rootfs/etc/nginx/lua/util/same_site.lua | 36 ++++++++++ 11 files changed, 249 insertions(+), 6 deletions(-) create mode 100644 docs/examples/affinity/cookie/ingress-samesite.yaml create mode 100644 rootfs/etc/nginx/lua/test/util/same_site_test.lua create mode 100644 rootfs/etc/nginx/lua/util/same_site.lua diff --git a/docs/examples/affinity/cookie/README.md b/docs/examples/affinity/cookie/README.md index a995ca2f4..f5a869c27 100644 --- a/docs/examples/affinity/cookie/README.md +++ b/docs/examples/affinity/cookie/README.md @@ -12,6 +12,8 @@ Session affinity can be configured using the following annotations: |nginx.ingress.kubernetes.io/affinity-mode|The affinity mode defines how sticky a session is. Use `balanced` to redistribute some sessions when scaling pods or `persistent` for maximum stickyness.|`balanced` (default) or `persistent`| |nginx.ingress.kubernetes.io/session-cookie-name|Name of the cookie that will be created|string (defaults to `INGRESSCOOKIE`)| |nginx.ingress.kubernetes.io/session-cookie-path|Path that will be set on the cookie (required if your [Ingress paths][ingress-paths] use regular expressions)|string (defaults to the currently [matched path][ingress-paths])| +|nginx.ingress.kubernetes.io/session-cookie-samesite|SameSite attribute to apply to the cookie|Browser accepted values are `None`, `Lax`, and `Strict`| +|nginx.ingress.kubernetes.io/session-cookie-conditional-samesite-none|Will omit `SameSite=None` attribute for older browsers which reject the more-recently defined `SameSite=None` value|`"true"` or `"false"` |nginx.ingress.kubernetes.io/session-cookie-max-age|Time until the cookie expires, corresponds to the `Max-Age` cookie directive|number of seconds| |nginx.ingress.kubernetes.io/session-cookie-expires|Legacy version of the previous annotation for compatibility with older browsers, generates an `Expires` cookie directive by adding the seconds to the current date|number of seconds| |nginx.ingress.kubernetes.io/session-cookie-change-on-failure|When set to `false` nginx ingress will send request to upstream pointed by sticky cookie even if previous attempt failed. When set to `true` and previous attempt failed, sticky cookie will be changed to point to another upstream.|`true` or `false` (defaults to `false`)| diff --git a/docs/examples/affinity/cookie/ingress-samesite.yaml b/docs/examples/affinity/cookie/ingress-samesite.yaml new file mode 100644 index 000000000..42d1c2e2d --- /dev/null +++ b/docs/examples/affinity/cookie/ingress-samesite.yaml @@ -0,0 +1,40 @@ +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: cookie-samesite-none + annotations: + nginx.ingress.kubernetes.io/affinity: "cookie" + nginx.ingress.kubernetes.io/session-cookie-name: "SSNONE" + nginx.ingress.kubernetes.io/session-cookie-expires: "172800" + nginx.ingress.kubernetes.io/session-cookie-max-age: "172800" + nginx.ingress.kubernetes.io/session-cookie-samesite: "None" + nginx.ingress.kubernetes.io/session-cookie-conditional-samesite-none: "true" # omits SameSite=None for older browsers which reject cookies with SameSite=None +spec: + rules: + - host: stickyingress-samesite-none.example.com + http: + paths: + - backend: + serviceName: http-svc + servicePort: 80 + path: / +--- +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: cookie-samesite-strict + annotations: + nginx.ingress.kubernetes.io/affinity: "cookie" + nginx.ingress.kubernetes.io/session-cookie-name: "STRICTCOOKIENAME" + nginx.ingress.kubernetes.io/session-cookie-expires: "172800" + nginx.ingress.kubernetes.io/session-cookie-max-age: "172800" + nginx.ingress.kubernetes.io/session-cookie-samesite: "Strict" +spec: + rules: + - host: stickyingress-samesite-strict.example.com + http: + paths: + - backend: + serviceName: http-svc + servicePort: 80 + path: / diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 3a00afa26..3bc2c45b3 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -84,6 +84,8 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/session-cookie-name](#cookie-affinity)|string| |[nginx.ingress.kubernetes.io/session-cookie-path](#cookie-affinity)|string| |[nginx.ingress.kubernetes.io/session-cookie-change-on-failure](#cookie-affinity)|"true" or "false"| +|[nginx.ingress.kubernetes.io/session-cookie-samesite](#cookie-affinity)|string| +|[nginx.ingress.kubernetes.io/session-cookie-conditional-samesite-none](#cookie-affinity)|"true" or "false"| |[nginx.ingress.kubernetes.io/ssl-redirect](#server-side-https-enforcement-through-redirect)|"true" or "false"| |[nginx.ingress.kubernetes.io/ssl-passthrough](#ssl-passthrough)|"true" or "false"| |[nginx.ingress.kubernetes.io/upstream-hash-by](#custom-nginx-upstream-hashing)|string| @@ -169,6 +171,7 @@ If you use the ``cookie`` affinity type you can also specify the name of the coo The NGINX annotation `nginx.ingress.kubernetes.io/session-cookie-path` defines the path that will be set on the cookie. This is optional unless the annotation `nginx.ingress.kubernetes.io/use-regex` is set to true; Session cookie paths do not support regex. +Use `nginx.ingress.kubernetes.io/session-cookie-samesite` to apply a `SameSite` attribute to the sticky cookie. Browser accepted values are `None`, `Lax`, and `Strict`. Some older browsers reject cookies with the more-recently-defined `SameSite=None`. To omit `SameSite=None` from these older browsers, add the annotation `nginx.ingress.kubernetes.io/session-cookie-conditional-samesite-none: "true"`. ### Authentication diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index d6ce54f5f..ac2a287c4 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -46,6 +46,12 @@ const ( // This is used to control the cookie path when use-regex is set to true annotationAffinityCookiePath = "session-cookie-path" + // This is used to control the SameSite attribute of the cookie + annotationAffinityCookieSameSite = "session-cookie-samesite" + + // This is used to control whether SameSite=None should be conditionally applied based on the User-Agent + annotationAffinityCookieConditionalSameSiteNone = "session-cookie-conditional-samesite-none" + // This is used to control the cookie change after request failure annotationAffinityCookieChangeOnFailure = "session-cookie-change-on-failure" ) @@ -75,6 +81,10 @@ type Cookie struct { Path string `json:"path"` // Flag that allows cookie regeneration on request failure ChangeOnFailure bool `json:"changeonfailure"` + // SameSite attribute value + SameSite string `json:"samesite"` + // Flag that conditionally applies SameSite=None attribute on cookie if user agent accepts it. + ConditionalSameSiteNone bool `json:"conditional-samesite-none"` } // cookieAffinityParse gets the annotation values related to Cookie Affinity @@ -107,6 +117,16 @@ func (a affinity) cookieAffinityParse(ing *networking.Ingress) *Cookie { klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) } + cookie.SameSite, err = parser.GetStringAnnotation(annotationAffinityCookieSameSite, ing) + if err != nil { + klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieSameSite) + } + + cookie.ConditionalSameSiteNone, err = parser.GetBoolAnnotation(annotationAffinityCookieConditionalSameSiteNone, ing) + if err != nil { + klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieConditionalSameSiteNone) + } + cookie.ChangeOnFailure, err = parser.GetBoolAnnotation(annotationAffinityCookieChangeOnFailure, ing) if err != nil { klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieChangeOnFailure) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 48a60424c..07aa0ee80 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -592,6 +592,8 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in ups.SessionAffinity.CookieSessionAffinity.Expires = anns.SessionAffinity.Cookie.Expires ups.SessionAffinity.CookieSessionAffinity.MaxAge = anns.SessionAffinity.Cookie.MaxAge ups.SessionAffinity.CookieSessionAffinity.Path = cookiePath + ups.SessionAffinity.CookieSessionAffinity.SameSite = anns.SessionAffinity.Cookie.SameSite + ups.SessionAffinity.CookieSessionAffinity.ConditionalSameSiteNone = anns.SessionAffinity.Cookie.ConditionalSameSiteNone ups.SessionAffinity.CookieSessionAffinity.ChangeOnFailure = anns.SessionAffinity.Cookie.ChangeOnFailure locs := ups.SessionAffinity.CookieSessionAffinity.Locations diff --git a/internal/ingress/types.go b/internal/ingress/types.go index dfd527187..63714d739 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -144,12 +144,14 @@ type SessionAffinityConfig struct { // CookieSessionAffinity defines the structure used in Affinity configured by Cookies. // +k8s:deepcopy-gen=true type CookieSessionAffinity struct { - Name string `json:"name"` - Expires string `json:"expires,omitempty"` - MaxAge string `json:"maxage,omitempty"` - Locations map[string][]string `json:"locations,omitempty"` - Path string `json:"path,omitempty"` - ChangeOnFailure bool `json:"change_on_failure,omitempty"` + Name string `json:"name"` + Expires string `json:"expires,omitempty"` + MaxAge string `json:"maxage,omitempty"` + Locations map[string][]string `json:"locations,omitempty"` + Path string `json:"path,omitempty"` + SameSite string `json:"samesite,omitempty"` + ConditionalSameSiteNone bool `json:"conditional_samesite_none,omitempty"` + ChangeOnFailure bool `json:"change_on_failure,omitempty"` } // UpstreamHashByConfig described setting from the upstream-hash-by* annotations. diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index 6eba4c3c2..f9f119030 100644 --- a/internal/ingress/types_equals.go +++ b/internal/ingress/types_equals.go @@ -179,6 +179,12 @@ func (csa1 *CookieSessionAffinity) Equal(csa2 *CookieSessionAffinity) bool { if csa1.MaxAge != csa2.MaxAge { return false } + if csa1.SameSite != csa2.SameSite { + return false + } + if csa1.ConditionalSameSiteNone != csa2.ConditionalSameSiteNone { + return false + } return true } diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 91183caf7..3527f1f73 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -2,6 +2,7 @@ local balancer_resty = require("balancer.resty") local ck = require("resty.cookie") local ngx_balancer = require("ngx.balancer") local split = require("util.split") +local same_site = require("util.same_site") local _M = balancer_resty:new() local DEFAULT_COOKIE_NAME = "route" @@ -43,6 +44,20 @@ function _M.set_cookie(self, value) cookie_path = ngx.var.location_path end + local cookie_samesite = self.cookie_session_affinity.samesite + if cookie_samesite then + local cookie_conditional_samesite_none = self.cookie_session_affinity.conditional_samesite_none + if cookie_conditional_samesite_none + and cookie_samesite == "None" + and not same_site.same_site_none_compatible(ngx.var.http_user_agent) then + cookie_samesite = nil + end + end + + if cookie_samesite then + cookie_path = cookie_path .. "; SameSite=" .. cookie_samesite + end + local cookie_data = { key = self:cookie_name(), value = value, diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index 2a258ec0d..d967769e1 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -390,4 +390,70 @@ describe("Sticky", function() it("sets a cookie on the client", function() test(sticky_balanced) end) it("sets a cookie on the client", function() test(sticky_persistent) end) end) + + describe("SameSite settings", function() + local mocked_cookie_new = cookie.new + + before_each(function() + package.loaded["balancer.sticky_balanced"] = nil + package.loaded["balancer.sticky_persistent"] = nil + sticky_balanced = require("balancer.sticky_balanced") + sticky_persistent = require("balancer.sticky_persistent") + end) + + after_each(function() + cookie.new = mocked_cookie_new + end) + + local function test_set_cookie(sticky, samesite, conditional_samesite_none, expected_path) + local s = {} + cookie.new = function(self) + local cookie_instance = { + set = function(self, payload) + assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name) + assert.equal(payload.path, expected_path) + assert.equal(payload.domain, nil) + assert.equal(payload.httponly, true) + assert.equal(payload.secure, false) + return true, nil + end, + get = function(k) return false end, + } + s = spy.on(cookie_instance, "set") + return cookie_instance, false + end + local b = get_test_backend() + b.sessionAffinityConfig.cookieSessionAffinity.locations = {} + b.sessionAffinityConfig.cookieSessionAffinity.locations["test.com"] = {"/"} + b.sessionAffinityConfig.cookieSessionAffinity.samesite = samesite + b.sessionAffinityConfig.cookieSessionAffinity.conditional_samesite_none = conditional_samesite_none + local sticky_balancer_instance = sticky:new(b) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + end + + it("returns a cookie with SameSite=Strict when user specifies samesite strict", function() + test_set_cookie(sticky_balanced, "Strict", false, "/; SameSite=Strict") + end) + it("returns a cookie with SameSite=Strict when user specifies samesite strict and conditional samesite none", function() + test_set_cookie(sticky_balanced, "Strict", true, "/; SameSite=Strict") + end) + it("returns a cookie with SameSite=Lax when user specifies samesite lax", function() + test_set_cookie(sticky_balanced, "Lax", false, "/; SameSite=Lax") + end) + it("returns a cookie with SameSite=Lax when user specifies samesite lax and conditional samesite none", function() + test_set_cookie(sticky_balanced, "Lax", true, "/; SameSite=Lax") + end) + it("returns a cookie with SameSite=None when user specifies samesite None", function() + test_set_cookie(sticky_balanced, "None", false, "/; SameSite=None") + end) + it("returns a cookie with SameSite=None when user specifies samesite None and conditional samesite none with supported user agent", function() + mock_ngx({ var = { location_path = "/", host = "test.com" , http_user_agent = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.2704.103 Safari/537.36"} }) + test_set_cookie(sticky_balanced, "None", true, "/; SameSite=None") + end) + it("returns a cookie without SameSite=None when user specifies samesite None and conditional samesite none with unsupported user agent", function() + mock_ngx({ var = { location_path = "/", host = "test.com" , http_user_agent = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36"} }) + test_set_cookie(sticky_balanced, "None", true, "/") + end) + end) end) diff --git a/rootfs/etc/nginx/lua/test/util/same_site_test.lua b/rootfs/etc/nginx/lua/test/util/same_site_test.lua new file mode 100644 index 000000000..341f2bb99 --- /dev/null +++ b/rootfs/etc/nginx/lua/test/util/same_site_test.lua @@ -0,0 +1,51 @@ +describe("same_site_compatible_test", function() + it("returns false for chrome 4", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2704.103 Safari/537.36")) + end) + it("returns false for chrome 5", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2704.103 Safari/537.36")) + end) + it("returns false for chrome 6", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2704.103 Safari/537.36")) + end) + it("returns false for iPhone OS 12", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/602.1.50 (KHTML, like Gecko) CriOS/56.0.2924.75 Mobile/14E5239e Safari/602.1")) + end) + it("returns false for iPad OS 12", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (iPad; CPU OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0 Mobile/15E148 Safari/604.1")) + end) + it("returns false for Mac 10.14 Safari", function() + local same_site = require("util.same_site") + assert.False(same_site.same_site_none_compatible("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.2 Safari/605.1.15")) + end) + + it("returns true for chrome 7", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.2704.103 Safari/537.36")) + end) + it("returns true for chrome 8", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.2704.103 Safari/537.36")) + end) + it("returns true for iPhone OS 13", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (iPhone; CPU iPhone OS 13_0 like Mac OS X) AppleWebKit/602.1.50 (KHTML, like Gecko) CriOS/56.0.2924.75 Mobile/14E5239e Safari/602.1")) + end) + it("returns true for iPad OS 13", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (iPad; CPU OS 13_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0 Mobile/15E148 Safari/604.1")) + end) + it("returns true for Mac 10.15 Safari", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.4 Safari/605.1.15")) + end) + it("returns true for Mac 10.14 Chrome", function() + local same_site = require("util.same_site") + assert.True(same_site.same_site_none_compatible("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.117 Safari/537.36")) + end) +end) diff --git a/rootfs/etc/nginx/lua/util/same_site.lua b/rootfs/etc/nginx/lua/util/same_site.lua new file mode 100644 index 000000000..735bd132f --- /dev/null +++ b/rootfs/etc/nginx/lua/util/same_site.lua @@ -0,0 +1,36 @@ +local _M = {} + +-- determines whether to apply a SameSite=None attribute +-- to a cookie, based on the user agent. +-- returns: boolean +-- +-- Chrome 80 treating third-party cookies as SameSite=Strict +-- if SameSite is missing. Certain old browsers don't recognize +-- SameSite=None and will reject cookies entirely bearing SameSite=None. +-- This creates a situation where fixing things for +-- Chrome >= 80 breaks things for old browsers. +-- This function compares the user agent against known +-- browsers which will reject SameSite=None cookies. +-- reference: https://www.chromium.org/updates/same-site/incompatible-clients +function _M.same_site_none_compatible(user_agent) + if string.match(user_agent, "Chrome/4") then + return false + elseif string.match(user_agent, "Chrome/5") then + return false + elseif string.match(user_agent, "Chrome/6") then + return false + elseif string.match(user_agent, "CPU iPhone OS 12") then + return false + elseif string.match(user_agent, "iPad; CPU OS 12") then + return false + elseif string.match(user_agent, "Macintosh") + and string.match(user_agent, "Intel Mac OS X 10_14") + and string.match(user_agent, "Safari") + and not string.match(user_agent, "Chrome") then + return false + end + + return true +end + +return _M