Add SameSite support - omit None for old browsers #4949

BrianKopp · 2020-01-21T21:33:51Z

This PR includes two main additions. First, a session-cookie-samesite attribute allows configuration of the cookie SameSite attribute with the provided value. Second, if SameSite=None is specified and session-cookie-conditional-samesite-none = "true", then the code which generates the cookie checks the client's User-Agent header and omits the SameSite=None cookie attribute for old browsers which would otherwise reject a SameSite=None. The None option is relatively recent, and these old browsers simply reject cookies with this attribute.

Supersedes #4878

What this PR does / why we need it:

This PR solves the impending Chrome 80 release in February which will treat all third-party cookies as SameSite=Strict if they omit the SameSite cookie attribute. This would otherwise break stickiness for users making third-party requests. E.g. you have an app on hostone.com, but you want stickiness for your api which is on hosttwo.com. These two hosts are third-party, and without SameSite=None on the hosttwo.com response, Chrome 80 won't send the hosttwo.com cookie to your backend which is expecting stickiness.

Source: https://www.chromium.org/updates/same-site

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Unit tests have been added to test the conditional SameSite=None logic against various User-Agent headers.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2020-01-21T21:34:00Z

Hi @BrianKopp. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aledbf · 2020-01-21T23:29:17Z

/ok-to-test

aledbf · 2020-01-21T23:29:32Z

/assign @ElvinEfendi

aledbf · 2020-01-21T23:34:05Z

@BrianKopp please squash the commits

docs/development.md

aledbf · 2020-01-22T12:12:30Z

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"`


Why two annotations?

session-cookie-samesite: "Lax" and session-cookie-samesite: "Strict" are perfectly valid inputs instead of session-cookie-samesite: "None". The need for conditionally applying the SameSite=None cookie attribute is that some older browsers reject SameSite=None cookies because the older browsers don't know about SameSite=None, since that's a newer specification that came out after Lax & Strict. The session-cookie-conditional-samesite-none flag allows users to turn on or off the SameSite=None User-Agent filtering functionality.

aledbf · 2020-01-22T12:13:07Z

internal/ingress/annotations/sessionaffinity/main.go

+	// 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"`


conditional_samesite_none -> conditional-samesite-none

This may be a dumb question, but how does this relate to... the CookieSessionAffinity type struct in internal/ingress/types.go line 146ish?

rootfs/etc/nginx/lua/util/same_site.lua

BrianKopp · 2020-01-22T20:23:30Z

/test pull-ingress-nginx-lualint

BrianKopp · 2020-01-22T20:25:02Z

/test pull-ingress-nginx-e2e-1-15

codecov-io · 2020-01-22T20:28:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5d05e19). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master    #4949   +/-   ##
=========================================
  Coverage          ?   58.58%           
=========================================
  Files             ?       88           
  Lines             ?     6777           
  Branches          ?        0           
=========================================
  Hits              ?     3970           
  Misses            ?     2372           
  Partials          ?      435

Impacted Files	Coverage Δ
internal/ingress/types.go	`0% <ø> (ø)`
internal/ingress/controller/controller.go	`50.18% <0%> (ø)`
internal/ingress/types_equals.go	`17.19% <0%> (ø)`
...ternal/ingress/annotations/sessionaffinity/main.go	`58.82% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d05e19...1b52339. Read the comment docs.

BrianKopp · 2020-01-23T17:49:20Z

/retest

ElvinEfendi · 2020-01-25T18:31:21Z

build/run-in-docker.sh

@@ -25,6 +25,10 @@ set -o pipefail
 # temporal directory for the /etc/ingress-controller directory
 INGRESS_VOLUME=$(mktemp -d)

+if [[ "$OSTYPE" == darwin* ]]; then
+  INGRESS_VOLUME=/private$INGRESS_VOLUME
+fi


ElvinEfendi · 2020-01-25T18:43:37Z

rootfs/etc/nginx/lua/balancer/sticky.lua

+
+  if cookie_samesite then
+    cookie_path = cookie_path .. "; SameSite=" .. cookie_samesite
+  end


The lua-resty-cookie library has native support for samesite, please use that attribute instead of misusing path attribute. All you need to do is: samesite = cookie_samesite, below after secure = ngx.var.https == "on",.

Would it be better to add a TODO here to fix this after that library adds SameSite=None support?

FYI the upstream change was merged a day or two ago.

@RulerOf yes, and the nginx base image already contains such version #5011
Now, someone needs to update the lua code :)

Pull requests are welcome

Will do both of these this weekend!

Drafted, will poke on tests later on this weekend. #5040

ElvinEfendi · 2020-01-25T18:48:54Z

rootfs/etc/nginx/lua/balancer/sticky.lua

+    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


Looking at cloudflare/lua-resty-cookie@8f41439#diff-8944ed02502a8a0a1f17e24e9b28c7a7R145, it seems like the library we use won't let us pass "None" for samesite. But given the new Chrome change you linked we should be able to set samesite to "Nonde".

I think I'd rather fix this in the upstream library than hack it here.

Therefore in the meantime I suggest we comply with what the upstream library provides, i.e not support "None" value at all. Then we patch the upstream library to support this new Chrome change and come back here to reflect that.

Yes, I'd rather fix it upstream as well. There is a currently a PR to that library to add support for SameSite=None, added 11 days ago. cloudflare/lua-resty-cookie#32

Unfortunately the library hasn't been updated in 3 years. Of the 5 contributors, only 1 has an active current github history.

According to Google Chrome's pages, version 80 is set to ship Feb 4th with 50% of users being on the new SameSite settings. I'm not sure we can wait around for a PR to go through on a repo that hasn't been touched in 3 years. I'm happy to come back through and change this to use the samesite argument in the library once it accepts it. What do you think?

https://www.chromestatus.com/features/schedule
https://www.chromium.org/updates/same-site

…ckwards compatibility

BrianKopp · 2020-01-29T22:53:06Z

/retest

aledbf · 2020-01-29T23:41:41Z

/retest

aledbf · 2020-01-30T00:37:16Z

/retest

aledbf · 2020-01-30T00:40:07Z

/retest

aledbf · 2020-01-30T01:22:29Z

/retest

aledbf · 2020-01-30T01:56:08Z

/retest

ElvinEfendi · 2020-01-31T03:28:32Z

docs/user-guide/nginx-configuration/annotations.md

@@ -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"`.


Can you make it easier for users reading this doc to understand how the "older browsers" is defined?

ElvinEfendi · 2020-01-31T03:30:45Z

Thanks for the patch @BrianKopp! I hope the PR in the upstream repository gets fixed soon and we revisit this. Otherwise I think we should fork and use our own version of lua-resty-cookie.

I don't want to block this PR for #4949 (comment), but would be great if that gets addressed in a subsequent PR.

/lgtm

k8s-ci-robot · 2020-01-31T03:31:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BrianKopp, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2020-01-31T11:15:55Z

/retest

RulerOf · 2020-02-13T16:53:32Z

rootfs/etc/nginx/lua/util/same_site.lua

+-- 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)


@BrianKopp I lifted this code to implement a fix on one of my apps and ran into a problem. We have some idiotic code that apparently doesn't send a User-Agent header. Running the request through this library causes a 502.

I don't know if there are safeguards in place for this but I thought I should warn you.

Awesome thank you for pointing this out! I've added handling for that here #5091

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 21, 2020

k8s-ci-robot requested review from aledbf and cmluciano January 21, 2020 21:35

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 21, 2020

BrianKopp marked this pull request as ready for review January 21, 2020 22:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2020

k8s-ci-robot assigned ElvinEfendi Jan 21, 2020

aledbf reviewed Jan 22, 2020

View reviewed changes

docs/development.md Outdated Show resolved Hide resolved

aledbf reviewed Jan 22, 2020

View reviewed changes

rootfs/etc/nginx/lua/util/same_site.lua Show resolved Hide resolved

BrianKopp force-pushed the same-site branch from 766fdfb to c3e9845 Compare January 22, 2020 20:20

ElvinEfendi reviewed Jan 25, 2020

View reviewed changes

Add SameSite=None support and conditionally omit SameSite=None for ba…

1b52339

…ckwards compatibility

BrianKopp force-pushed the same-site branch from c3e9845 to 1b52339 Compare January 29, 2020 21:30

ElvinEfendi reviewed Jan 31, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2020

k8s-ci-robot merged commit beef9fa into kubernetes:master Jan 31, 2020

BrianKopp mentioned this pull request Feb 8, 2020

Update documentation and remove hack fixed by upstream cookie library #5040

Merged

8 tasks

RulerOf reviewed Feb 13, 2020

View reviewed changes

BrianKopp mentioned this pull request Feb 16, 2020

Add case for when user agent is nil #5091

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SameSite support - omit None for old browsers #4949

Add SameSite support - omit None for old browsers #4949

BrianKopp commented Jan 21, 2020 •

edited

Loading

k8s-ci-robot commented Jan 21, 2020

aledbf commented Jan 21, 2020

aledbf commented Jan 21, 2020

aledbf commented Jan 21, 2020

aledbf Jan 22, 2020

BrianKopp Jan 22, 2020

aledbf Jan 22, 2020

BrianKopp Jan 22, 2020

BrianKopp commented Jan 22, 2020

BrianKopp commented Jan 22, 2020

codecov-io commented Jan 22, 2020 •

edited

Loading

BrianKopp commented Jan 23, 2020

ElvinEfendi Jan 25, 2020

ElvinEfendi Jan 25, 2020

BrianKopp Jan 26, 2020

RulerOf Feb 6, 2020

aledbf Feb 6, 2020

BrianKopp Feb 8, 2020

BrianKopp Feb 8, 2020

ElvinEfendi Jan 25, 2020 •

edited

Loading

BrianKopp Jan 26, 2020

BrianKopp commented Jan 29, 2020

aledbf commented Jan 29, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

ElvinEfendi Jan 31, 2020

ElvinEfendi commented Jan 31, 2020

k8s-ci-robot commented Jan 31, 2020

aledbf commented Jan 31, 2020

RulerOf Feb 13, 2020

BrianKopp Feb 16, 2020

		@@ -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"`.

Add SameSite support - omit None for old browsers #4949

Add SameSite support - omit None for old browsers #4949

Conversation

BrianKopp commented Jan 21, 2020 • edited Loading

What this PR does / why we need it:

Types of changes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Jan 21, 2020

aledbf commented Jan 21, 2020

aledbf commented Jan 21, 2020

aledbf commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianKopp commented Jan 22, 2020

BrianKopp commented Jan 22, 2020

codecov-io commented Jan 22, 2020 • edited Loading

Codecov Report

BrianKopp commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Jan 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianKopp commented Jan 29, 2020

aledbf commented Jan 29, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

aledbf commented Jan 30, 2020

Choose a reason for hiding this comment

ElvinEfendi commented Jan 31, 2020

k8s-ci-robot commented Jan 31, 2020

aledbf commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianKopp commented Jan 21, 2020 •

edited

Loading

codecov-io commented Jan 22, 2020 •

edited

Loading

ElvinEfendi Jan 25, 2020 •

edited

Loading