From f2e743f561963f140e39f66bd2480d1ab2e68ab6 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Thu, 2 Sep 2021 00:23:40 +0200 Subject: [PATCH] feat: add session-cookie-secure annotation (#7399) --- docs/examples/affinity/cookie/README.md | 1 + .../affinity/cookie/ingress-samesite.yaml | 1 + .../annotations/sessionaffinity/main.go | 11 ++++++ .../annotations/sessionaffinity/main_test.go | 5 +++ internal/ingress/controller/controller.go | 1 + internal/ingress/types.go | 1 + internal/ingress/types_equals.go | 3 ++ rootfs/etc/nginx/lua/balancer/sticky.lua | 7 +++- .../nginx/lua/test/balancer/sticky_test.lua | 38 ++++++++++--------- 9 files changed, 50 insertions(+), 18 deletions(-) diff --git a/docs/examples/affinity/cookie/README.md b/docs/examples/affinity/cookie/README.md index 4f93fc0f8..b567c44c0 100644 --- a/docs/examples/affinity/cookie/README.md +++ b/docs/examples/affinity/cookie/README.md @@ -12,6 +12,7 @@ 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 stickiness.|`balanced` (default) or `persistent`| |nginx.ingress.kubernetes.io/affinity-canary-behavior|Defines session affinity behavior of canaries. By default the behavior is `sticky`, and canaries respect session affinity configuration. Set this to `legacy` to restore original canary behavior, when session affinity parameters were not respected.|`sticky` (default) or `legacy`| |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-secure|Set the cookie as secure regardless the protocol of the incoming request|`"true"` or `"false"`| |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"` diff --git a/docs/examples/affinity/cookie/ingress-samesite.yaml b/docs/examples/affinity/cookie/ingress-samesite.yaml index b3f8f4b20..edfa1b653 100644 --- a/docs/examples/affinity/cookie/ingress-samesite.yaml +++ b/docs/examples/affinity/cookie/ingress-samesite.yaml @@ -5,6 +5,7 @@ metadata: annotations: nginx.ingress.kubernetes.io/affinity: "cookie" nginx.ingress.kubernetes.io/session-cookie-name: "SSNONE" + nginx.ingress.kubernetes.io/session-cookie-secure: "true" 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" diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 358be1442..9c4d1d2bc 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -37,6 +37,10 @@ const ( defaultAffinityCookieName = "INGRESSCOOKIE" + // This is used to force the Secure flag on the cookie even if the + // incoming request is not secured. (https://github.com/kubernetes/ingress-nginx/issues/6812) + annotationAffinityCookieSecure = "session-cookie-secure" + // This is used to control the cookie expires, its value is a number of seconds until the // cookie expires annotationAffinityCookieExpires = "session-cookie-expires" @@ -85,6 +89,8 @@ type Cookie struct { Path string `json:"path"` // Flag that allows cookie regeneration on request failure ChangeOnFailure bool `json:"changeonfailure"` + // Secure flag to be set + Secure bool `json:"secure"` // SameSite attribute value SameSite string `json:"samesite"` // Flag that conditionally applies SameSite=None attribute on cookie if user agent accepts it. @@ -126,6 +132,11 @@ func (a affinity) cookieAffinityParse(ing *networking.Ingress) *Cookie { klog.V(3).InfoS("Invalid or no annotation value found. Ignoring", "ingress", klog.KObj(ing), "annotation", annotationAffinityCookieSameSite) } + cookie.Secure, err = parser.GetBoolAnnotation(annotationAffinityCookieSecure, ing) + if err != nil { + klog.V(3).InfoS("Invalid or no annotation value found. Ignoring", "ingress", klog.KObj(ing), "annotation", annotationAffinityCookieSecure) + } + cookie.ConditionalSameSiteNone, err = parser.GetBoolAnnotation(annotationAffinityCookieConditionalSameSiteNone, ing) if err != nil { klog.V(3).InfoS("Invalid or no annotation value found. Ignoring", "ingress", klog.KObj(ing), "annotation", annotationAffinityCookieConditionalSameSiteNone) diff --git a/internal/ingress/annotations/sessionaffinity/main_test.go b/internal/ingress/annotations/sessionaffinity/main_test.go index 98f0a1eb6..65d11ac2d 100644 --- a/internal/ingress/annotations/sessionaffinity/main_test.go +++ b/internal/ingress/annotations/sessionaffinity/main_test.go @@ -79,6 +79,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) { data[parser.GetAnnotationWithPrefix(annotationAffinityCookieMaxAge)] = "3000" data[parser.GetAnnotationWithPrefix(annotationAffinityCookiePath)] = "/foo" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieChangeOnFailure)] = "true" + data[parser.GetAnnotationWithPrefix(annotationAffinityCookieSecure)] = "true" ing.SetAnnotations(data) affin, _ := NewParser(&resolver.Mock{}).Parse(ing) @@ -114,4 +115,8 @@ func TestIngressAffinityCookieConfig(t *testing.T) { if !nginxAffinity.Cookie.ChangeOnFailure { t.Errorf("expected change of failure parameter set to true but returned %v", nginxAffinity.Cookie.ChangeOnFailure) } + + if !nginxAffinity.Cookie.Secure { + t.Errorf("expected secure parameter set to true but returned %v", nginxAffinity.Cookie.Secure) + } } diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 72a1afa63..1fd22079b 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -667,6 +667,7 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in ups.SessionAffinity.CookieSessionAffinity.Name = anns.SessionAffinity.Cookie.Name ups.SessionAffinity.CookieSessionAffinity.Expires = anns.SessionAffinity.Cookie.Expires ups.SessionAffinity.CookieSessionAffinity.MaxAge = anns.SessionAffinity.Cookie.MaxAge + ups.SessionAffinity.CookieSessionAffinity.Secure = anns.SessionAffinity.Cookie.Secure ups.SessionAffinity.CookieSessionAffinity.Path = cookiePath ups.SessionAffinity.CookieSessionAffinity.SameSite = anns.SessionAffinity.Cookie.SameSite ups.SessionAffinity.CookieSessionAffinity.ConditionalSameSiteNone = anns.SessionAffinity.Cookie.ConditionalSameSiteNone diff --git a/internal/ingress/types.go b/internal/ingress/types.go index a1cbdf685..033fa9cc4 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -155,6 +155,7 @@ type CookieSessionAffinity struct { Expires string `json:"expires,omitempty"` MaxAge string `json:"maxage,omitempty"` Locations map[string][]string `json:"locations,omitempty"` + Secure bool `json:"secure,omitempty"` Path string `json:"path,omitempty"` SameSite string `json:"samesite,omitempty"` ConditionalSameSiteNone bool `json:"conditional_samesite_none,omitempty"` diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index 87af2bcfc..18ffa9042 100644 --- a/internal/ingress/types_equals.go +++ b/internal/ingress/types_equals.go @@ -182,6 +182,9 @@ func (csa1 *CookieSessionAffinity) Equal(csa2 *CookieSessionAffinity) bool { if csa1.SameSite != csa2.SameSite { return false } + if csa1.Secure != csa2.Secure { + return false + } if csa1.ConditionalSameSiteNone != csa2.ConditionalSameSiteNone { return false } diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 63f3c6785..3440d86bd 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -87,13 +87,18 @@ function _M.set_cookie(self, value) end end + local cookie_secure = self.cookie_session_affinity.secure + if cookie_secure == nil then + cookie_secure = ngx.var.https == "on" + end + local cookie_data = { key = self:cookie_name(), value = value .. COOKIE_VALUE_DELIMITER .. self.backend_key, path = cookie_path, httponly = true, samesite = cookie_samesite, - secure = ngx.var.https == "on", + secure = cookie_secure, } if self.cookie_session_affinity.expires and self.cookie_session_affinity.expires ~= "" then diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index a0c0ae54f..80d0c0d0e 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -422,7 +422,7 @@ describe("Sticky", function() cookie.new = mocked_cookie_new end) - local function test_set_cookie_with(sticky_balancer_type, samesite, conditional_samesite_none, expected_path, expected_samesite) + local function test_set_cookie_with(sticky_balancer_type, samesite, conditional_samesite_none, expected_path, expected_samesite, secure, expected_secure) local s = {} cookie.new = function(self) local cookie_instance = { @@ -432,7 +432,7 @@ describe("Sticky", function() assert.equal(payload.samesite, expected_samesite) assert.equal(payload.domain, nil) assert.equal(payload.httponly, true) - assert.equal(payload.secure, false) + assert.equal(payload.secure, expected_secure) return true, nil end, get = function(k) return false end, @@ -445,57 +445,61 @@ describe("Sticky", function() b.sessionAffinityConfig.cookieSessionAffinity.locations["test.com"] = {"/"} b.sessionAffinityConfig.cookieSessionAffinity.samesite = samesite b.sessionAffinityConfig.cookieSessionAffinity.conditional_samesite_none = conditional_samesite_none + b.sessionAffinityConfig.cookieSessionAffinity.secure = secure local sticky_balancer_instance = sticky_balancer_type: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_with(sticky_balanced, "Strict", false, "/", "Strict") + it("returns a secure cookie with SameSite=Strict when user specifies samesite strict and secure=true", function() + test_set_cookie_with(sticky_balanced, "Lax", false, "/", "Lax", true, true) end) it("returns a cookie with SameSite=Strict when user specifies samesite strict and conditional samesite none", function() - test_set_cookie_with(sticky_balanced, "Strict", true, "/", "Strict") + test_set_cookie_with(sticky_balanced, "Strict", true, "/", "Strict", nil, false) end) it("returns a cookie with SameSite=Lax when user specifies samesite lax", function() - test_set_cookie_with(sticky_balanced, "Lax", false, "/", "Lax") + test_set_cookie_with(sticky_balanced, "Lax", false, "/", "Lax", nil, false) end) it("returns a cookie with SameSite=Lax when user specifies samesite lax and conditional samesite none", function() - test_set_cookie_with(sticky_balanced, "Lax", true, "/", "Lax") + test_set_cookie_with(sticky_balanced, "Lax", true, "/", "Lax", nil, false) end) it("returns a cookie with SameSite=None when user specifies samesite None", function() - test_set_cookie_with(sticky_balanced, "None", false, "/", "None") + test_set_cookie_with(sticky_balanced, "None", false, "/", "None", nil, false) 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_with(sticky_balanced, "None", true, "/", "None") + test_set_cookie_with(sticky_balanced, "None", true, "/", "None", nil, false) 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_with(sticky_balanced, "None", true, "/", nil) + test_set_cookie_with(sticky_balanced, "None", true, "/", nil, nil, false) end) + it("returns a secure cookie with SameSite=Strict when user specifies samesite strict and secure=true", function() + test_set_cookie_with(sticky_persistent, "Lax", false, "/", "Lax", true, true) + end) it("returns a cookie with SameSite=Strict when user specifies samesite strict", function() - test_set_cookie_with(sticky_persistent, "Strict", false, "/", "Strict") + test_set_cookie_with(sticky_persistent, "Strict", false, "/", "Strict", nil, false) end) it("returns a cookie with SameSite=Strict when user specifies samesite strict and conditional samesite none", function() - test_set_cookie_with(sticky_persistent, "Strict", true, "/", "Strict") + test_set_cookie_with(sticky_persistent, "Strict", true, "/", "Strict", nil, false) end) it("returns a cookie with SameSite=Lax when user specifies samesite lax", function() - test_set_cookie_with(sticky_persistent, "Lax", false, "/", "Lax") + test_set_cookie_with(sticky_persistent, "Lax", false, "/", "Lax", nil, false) end) it("returns a cookie with SameSite=Lax when user specifies samesite lax and conditional samesite none", function() - test_set_cookie_with(sticky_persistent, "Lax", true, "/", "Lax") + test_set_cookie_with(sticky_persistent, "Lax", true, "/", "Lax", nil, false) end) it("returns a cookie with SameSite=None when user specifies samesite None", function() - test_set_cookie_with(sticky_persistent, "None", false, "/", "None") + test_set_cookie_with(sticky_persistent, "None", false, "/", "None", nil, false) 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_with(sticky_persistent, "None", true, "/", "None") + test_set_cookie_with(sticky_persistent, "None", true, "/", "None", nil, false) 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_with(sticky_persistent, "None", true, "/", nil) + test_set_cookie_with(sticky_persistent, "None", true, "/", nil, nil, false) end) end)