You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/06/16 15:21:11 UTC
[GitHub] [knox] moresandeep opened a new pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
moresandeep opened a new pull request #347:
URL: https://github.com/apache/knox/pull/347
# 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #347:
URL: https://github.com/apache/knox/pull/347
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] risdenk commented on a change in pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #347:
URL: https://github.com/apache/knox/pull/347#discussion_r440980461
##########
File path: gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java
##########
@@ -350,26 +350,37 @@ private long getExpiry() {
private void addJWTHadoopCookie(String original, JWT token) {
LOGGER.addingJWTCookie(token.toString());
- Cookie c = new Cookie(cookieName, token.toString());
- c.setPath("/");
+ /*
+ * In order to account for google chrome changing default value
+ * of SameSite from None to Lax we need to craft Set-Cookie
+ * header to prevent issues with hadoop-jwt cookie.
+ * NOTE: this would have been easier if javax.servlet.http.Cookie supported
+ * SameSite param. Change this back to Cookie impl. after
+ * SameSite header is supported by javax.servlet.http.Cookie.
+ */
+ final StringBuilder setCookie = new StringBuilder(50);
try {
- String domain = Urls.getDomainName(original, domainSuffix);
+ setCookie.append(cookieName).append('=').append(token.toString());
+ setCookie.append("; Path=/");
+ final String domain = Urls.getDomainName(original, domainSuffix);
if (domain != null) {
- c.setDomain(domain);
+ setCookie.append("; Domain=").append(domain);
}
- c.setHttpOnly(true);
+ setCookie.append("; HttpOnly");
if (secureOnly) {
- c.setSecure(true);
+ setCookie.append("; Secure");
}
if (maxAge != -1) {
- c.setMaxAge(maxAge);
+ setCookie.append("; Max-Age=").append(maxAge);
}
- response.addCookie(c);
+ setCookie.append("; SameSite=None");
+ response.setHeader("Set-Cookie", setCookie.toString());
Review comment:
Why is the cookie being created as a string?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #347:
URL: https://github.com/apache/knox/pull/347#discussion_r441037372
##########
File path: gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java
##########
@@ -350,26 +350,37 @@ private long getExpiry() {
private void addJWTHadoopCookie(String original, JWT token) {
LOGGER.addingJWTCookie(token.toString());
- Cookie c = new Cookie(cookieName, token.toString());
- c.setPath("/");
+ /*
+ * In order to account for google chrome changing default value
+ * of SameSite from None to Lax we need to craft Set-Cookie
+ * header to prevent issues with hadoop-jwt cookie.
+ * NOTE: this would have been easier if javax.servlet.http.Cookie supported
+ * SameSite param. Change this back to Cookie impl. after
+ * SameSite header is supported by javax.servlet.http.Cookie.
+ */
+ final StringBuilder setCookie = new StringBuilder(50);
try {
- String domain = Urls.getDomainName(original, domainSuffix);
+ setCookie.append(cookieName).append('=').append(token.toString());
+ setCookie.append("; Path=/");
+ final String domain = Urls.getDomainName(original, domainSuffix);
if (domain != null) {
- c.setDomain(domain);
+ setCookie.append("; Domain=").append(domain);
}
- c.setHttpOnly(true);
+ setCookie.append("; HttpOnly");
if (secureOnly) {
- c.setSecure(true);
+ setCookie.append("; Secure");
}
if (maxAge != -1) {
- c.setMaxAge(maxAge);
+ setCookie.append("; Max-Age=").append(maxAge);
}
- response.addCookie(c);
+ setCookie.append("; SameSite=None");
+ response.setHeader("Set-Cookie", setCookie.toString());
Review comment:
[javax.servlet.http.Cookie](https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/Cookie.html) class does not support `SameSite` property, there is no way to add a param hence the `Set-Header`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on pull request #347:
URL: https://github.com/apache/knox/pull/347#issuecomment-645158025
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`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on pull request #347: KNOX-2387 - SameSite fix for hadoop-jwt cookie
Posted by GitBox <gi...@apache.org>.
moresandeep commented on pull request #347:
URL: https://github.com/apache/knox/pull/347#issuecomment-645300250
> 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 ](https://www.theverge.com/2020/4/3/21207248/chrome-samesite-cookie-roll-back-update-privacy-settings)
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](https://support.okta.com/help/s/article/FAQ-How-Chrome-80-Update-for-SameSite-by-default-Potentially-Impacts-Your-Okta-Environment) 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org