Skip to content

KNOX-2387 - SameSite fix for hadoop-jwt cookie#347

Merged
moresandeep merged 1 commit intoapache:masterfrom
moresandeep:KNOX-2387_SameSite_Fix
Jun 17, 2020
Merged

KNOX-2387 - SameSite fix for hadoop-jwt cookie#347
moresandeep merged 1 commit intoapache:masterfrom
moresandeep:KNOX-2387_SameSite_Fix

Conversation

@moresandeep
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Update the Set-Cookie header for hadoop-jwt cookie to include SameSite=none parameter.

How was this patch tested?

This patch was tested on a local cluster.

}
response.addCookie(c);
setCookie.append("; SameSite=None");
response.setHeader("Set-Cookie", setCookie.toString());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the cookie being created as a string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javax.servlet.http.Cookie class does not support SameSite property, there is no way to add a param hence the Set-Header.

Copy link
Copy Markdown
Contributor

@lmccay lmccay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
+1

@smolnar82
Copy link
Copy Markdown
Contributor

So, as far as I understood Chrome made the default behavior more secure by setting the default to Lax. With this change, we blindly set this to None to be backward compatible. At least, I'd introduce a provider parameter for this purpose to allow end-users to control it like this:

  1. in the init() method I'd parse the newly introduced knoxsso.cookie.samesite and save it to a class member
  2. in addJWTHadoopCookie I'd check if it's set and use the custom value or default to None

@moresandeep
Copy link
Copy Markdown
Contributor Author

So, as far as I understood Chrome made the default behavior more secure by setting the default to Lax. With this change, we blindly set this to None to be backward compatible. At least, I'd introduce a provider parameter for this purpose to allow end-users to control it like this:

  1. in the init() method I'd parse the newly introduced knoxsso.cookie.samesite and save it to a class member
  2. in addJWTHadoopCookie I'd check if it's set and use the custom value or default to None

The history of this fix in chrome is terrible (atleast from test this fix), the update is rolled back for the time being (until Covid-19) because it was causing a lot of websites to break.
Details: Google is temporarily rolling back Chrome’s SameSite cookie requirements

By changing SameSite=none we are not making it insecure, this is why:

  1. This is a legit use-case for SameSite=none, we are a third-party cookie used for SSO login and this cookie is required for proper SSO functioning.
  2. This is how it works in FF currently so anyone using FF will be using SameSite=none.
  3. Okta which is an IdP updated it's cookies to SameSite=none.

By adding a param to let users control it IMO is not required as none will be the only accepted value here, any other value would break SSO.

This is some documentation on this "feature" - https://www.chromestatus.com/feature/5088147346030592
This is a good writeup on this issue - https://support.okta.com/help/s/article/FAQ-How-Chrome-80-Update-for-SameSite-by-default-Potentially-Impacts-Your-Okta-Environment

@moresandeep moresandeep merged commit d10e15d into apache:master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants