Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure SameSite attribute on session Cookies with Spring Session #15047

Closed
candrews opened this issue Oct 31, 2018 · 29 comments
Closed

Configure SameSite attribute on session Cookies with Spring Session #15047

candrews opened this issue Oct 31, 2018 · 29 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@candrews
Copy link
Contributor

candrews commented Oct 31, 2018

Currently, there's no way from application.properties to configure the Spring Session session cookie's SameSite attribute. It would be nice to be able to do that.

For consistency with the existing server.servlet.session.cookie properties, I suggest:
server.servlet.session.cookie.sameSite with a default value of "Lax" (to match Spring Session 2.1's behavior defined in DefaultCookieSerializer). A value of empty string would map to null (which results in DefaultCookieSerializer not setting the SameSite attribute on the cookie).

SessionAutoConfiguration would implement this behavior.

This setting would have no effect when Spring Session is not in use as no servlet containers currently expose a means by which to set the SameSite attribute on their session cookies (support for that can be added as containers gain that ability).

Note that this is likely to be increasingly used as the default session cookie in Spring Session 2.1 has the attribute SameSite=Lax (see spring-projects/spring-session#1005) which breaks SAML login, so anyone using SAML (such as via Spring Security SAML) is going to have to need to change this configuration:

https://groups.google.com/a/chromium.org/d/msg/security-dev/AxY6BpkkH9U/vgKbDm7rFgAJ

As a further enhancement, perhaps if Spring Security SAML is detected, server.servlet.session can be set to null by default instead of "Lax".

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 31, 2018
@philwebb
Copy link
Member

This looks like a sensible addition, but I'd rather scope it specifically to Spring Session rather than make it a general feature. I've no idea if/when javax.servlet.http.Cookie will get the equivalent support and if feels a bit odd that a server.servlet.session.samesite property would just be ignored.

I think we should add it under spring.session.cookie.samesite and probably offer an enum with OFF, STRICT and LAX as values.

@philwebb philwebb added type: enhancement A general enhancement status: ideal-for-contribution An issue that a contributor can help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 31, 2018
@philwebb philwebb added this to the 2.2.x milestone Oct 31, 2018
@mbhave
Copy link
Contributor

mbhave commented Oct 31, 2018

I don't know if it makes sense to just make the sameSite attribute configuration for Spring Session. There are other things that can be configured such as cookieMaxAge etc and we don't have configuration properties for those either under SessionProperties.

@philwebb
Copy link
Member

Oh yeah :(

Actually, I'm a little confused as to how you change the sameSite attribute at all. The org.springframework.session.config.annotation.web.http.SpringHttpSessionConfiguration class seems to create a DefaultCookieSerializer then call setters from javax.servlet.SessionCookieConfig. I guess we currently rely on that.

Looks like we'd need to register a CookieSerializer as a bean and do that createDefaultCookieSerializer work ourselves.

@philwebb philwebb added status: on-hold We can't start working on this issue yet for: team-attention An issue we'd like other members of the team to review and removed status: ideal-for-contribution An issue that a contributor can help us with labels Oct 31, 2018
@philwebb
Copy link
Member

@vpavic
Copy link
Contributor

vpavic commented Nov 1, 2018

I can chime in with Spring Session side of things regarding SameSite cookie attribute, and everything around it which might be relevant from the perspective of extending the Spring Session auto-configuration support.

In Spring Session's HttpSession (i.e. Servlet) integration, there is a concept of HttpSessionIdResolver, which is the strategy for correlating incoming HTTP request to session - by default, a cookie based implementation (CookieHttpSessionIdResolver) is used while we also offer header based implementation (HeaderHttpSessionIdResolver) out of the box.

With cookie based implementation, the concept of CookieSerializer comes into play as well. This is the component that's responsible for writing the session cookie, and which the SameSite related configuration property would ultimately be mapped to.

I'm describing all this to make it clear that any extension of Spring Boot's auto-configuration support for Spring Session as requested here would likely have to consider both HttpSessionIdResolver and CookieSerializer.

Spring Session's configuration facilities try to be customization friendly by doing the following things:

  • if user registers a bean of type HttpSessionIdResolver, we pick it up and use instead of the default cookie based one
  • if user registers a bean of type CookieSerializer, we pick it up and configure the default cookie based HttpSessionIdResolver to use it
  • we try to configure the default CookieSerializer based on SessionCookieConfig

So one can customize the SameSite attribute of session cookie registering DefaultCookieSerializer bean with DefaultCookieSerializer#setSameSite set to null (or even go a step further and register the desirable HttpSessionIdResolver bean).

Having said that, the proposed server.servlet.session.samesite doesn't really make sense, as SameSite is cookie related and therefore any property related to it should sit under server.servlet.session.cookie namespace. However, since server.servlet.session.cookie actually maps SessionCookieConfig this would be confusing as Servlet API provides no SameSite support at the moment.

For reactive side of the things, the entire story is completely out of Spring Session's scope as WebSessionIdResolver (which is the equivalent of Spring Session's HttpSessionIdResolver) is actually a part of Spring WebFlux.

Considering that Spring Session's configuration is fairly simple to customize on its own, as well as the complexity the would be required to add to auto-configuration support in Spring Boot (and which would in big part duplicate what Spring Session's own configuration already offers on its own), and that there's no support for SameSite attribute in Servlet API, I'd recommend against making any changes. If Servlet API catches up in some future release and adds support for SameSite, the problem will solve itself as we on Spring Session side would pick up the new property from SessionCookieConfig and configure the default serializer accordingly.

@candrews
Copy link
Contributor Author

candrews commented Nov 2, 2018

Can we add a SameSite property (the type being an enum having values for lax, strict, and none, defaulting to lax) to SessionCookieConfig now, but have it not do anything for session implementations that don't support the SameSite attribute?

Right now, it's supportable for Spring Session. Support can be added for base servlet, WebFlux, etc as those packages gain SameSite support.

@vpavic
Copy link
Contributor

vpavic commented Nov 2, 2018

SessionCookieConfig is a Servlet API thing.

@candrews
Copy link
Contributor Author

candrews commented Nov 2, 2018

Sorry, I meant org.springframework.boot.web.servlet.server.Session.Cookie (again, my apologies for the mistake). The point I was making is that support for what can be supported can be done now without having to wait until javax.servlet.SessionCookieConfig is updated with SameSite support.

@wilkinsona
Copy link
Member

That comes back to @vpavic's earlier point which I agree with:

However, since server.servlet.session.cookie actually maps SessionCookieConfig this would be confusing as Servlet API provides no SameSite support at the moment.

I wouldn't want a general server.servlet.* property to only work when you're using Spring Session or only work when you're using a particular embedded container.

@candrews
Copy link
Contributor Author

candrews commented Nov 2, 2018

Even if the intent is to work with the servlet spec when it's updated, meaning this would be a temporary situation?

@wilkinsona
Copy link
Member

Yes as things would be confusing in the interim. Furthermore, there’s no guarantee that it would be temporary as the Servlet spec is out of our control.

@philwebb
Copy link
Member

philwebb commented Nov 5, 2018

Considering that Spring Session's configuration is fairly simple to customize on its own, as well as the complexity the would be required to add to auto-configuration support in Spring Boot (and which would in big part duplicate what Spring Session's own configuration already offers on its own), and that there's no support for SameSite attribute in Servlet API, I'd recommend against making any changes.

I'm inclined to agree. I'm going to close this one for now and wait to see how any Servlet API spec changes work out.

@philwebb philwebb closed this as completed Nov 5, 2018
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: on-hold We can't start working on this issue yet type: enhancement A general enhancement labels Nov 5, 2018
@vpavic
Copy link
Contributor

vpavic commented Nov 5, 2018

Given this was declined, it seems that 2.2.x milestone was left on by mistake.

@snicoll snicoll removed this from the 2.2.x milestone Nov 5, 2018
@OrangeDog
Copy link
Contributor

OrangeDog commented Jul 19, 2019

As well as SAML, it appears the defaults also make it impossible to use Access-Control-Allow-Credentials.

For others looking for property-based configuration, simply add this bean.

@Bean
@ConfigurationProperties("spring.session.cookie")
public DefaultCookieSerializer cookieSerializer()  {
  return new DefaultCookieSerializer();
}

@blop
Copy link

blop commented Nov 4, 2019

Seems this is getting more urgent:
https://www.chromium.org/updates/same-site

See also:
jetty/jetty.project#4247
jakartaee/servlet#175

@philwebb philwebb added this to the 2.2.x milestone Nov 6, 2019
@bclozel
Copy link
Member

bclozel commented Nov 6, 2019

This is properly supported in Spring Framework as of 5.1.10, see spring-projects/spring-framework#23693 and spring-projects/spring-framework#20964 - we'll consider how this should be supported in MVC and WebFlux.

@lucwillems
Copy link

i'm current using the default embedded tomcat servlet , spring boot 2.1.8 with NO spring boot session. it seems tomcat can be configured to have a Rfc6265CookieProcessor with sameSite=Strict
like this

@Configuration
public class TomcatConfiguration {

    @Bean
    WebServerFactoryCustomizer<TomcatServletWebServerFactory> cookieProcessorCustomizer() {
        return new WebServerFactoryCustomizer<TomcatServletWebServerFactory>() {

            @Override
            public void customize(TomcatServletWebServerFactory tomcatServletWebServerFactory) {
                tomcatServletWebServerFactory.addContextCustomizers(new TomcatContextCustomizer() {
                    @Override
                    public void customize(Context context) {
                        Rfc6265CookieProcessor processor=new Rfc6265CookieProcessor();
                        processor.setSameSiteCookies("strict");
                        context.setCookieProcessor(processor);
                    }
                });
            }
        };
    }
}

one remark is that ALL cookies set by this tomcat instance will have sameSite=strict, if that's not what you want, you can extend the cookie processor class and add some custom logic for this.

@buckett
Copy link

buckett commented Nov 21, 2019

@lucwillems Thanks for the code snippet, I tried this but Rfc6265CookieProcessor doesn't output a SameSite attribute on the cookie if it's set to None. This means you can't use the class as is for outputting a SameSite=None attribute to tell Chrome to still send cookies on cross domain requests after Chrome 80 (https://www.chromestatus.com/feature/5088147346030592)

@rougou
Copy link

rougou commented Jan 9, 2020

I tried this but Rfc6265CookieProcessor doesn't output a SameSite attribute on the cookie if it's set to None.

As you may know, this bug was already fixed at the time of your post, and is now released.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63865
apache/tomcat@ec782a0#diff-1f67ea28105562c05982ddba45cc0527

@OrangeDog
Copy link
Contributor

I feel a headache coming on...
It looks like static SameSite configuration isn't going to be sufficient.

@rougou
Copy link

rougou commented Jan 29, 2020

Yes, in particular Apple refuses to make a patch for iOS 12, so setting sameSite to None in all cases won't cut it.
Ideally DefaultCookieSerializer would allow you to override the sameSite value per request. For a work project, we got around this by making a separate DefaultCookieSerializer instance per sameSite type and switching between them in writeCookieValue().

@driverpt
Copy link

driverpt commented Feb 6, 2020

FYI, Chrome 80 has now rolled out

@buckett
Copy link

buckett commented Feb 6, 2020

Although the SameSite feature is being released more slowly: https://www.chromium.org/updates/same-site

@samuelstein
Copy link

An easy way around this problem is to use org.springframework.http.ResponseCookie class. The class has a nice builder with samesite property.
Then you can add the cookie to the response header:
response.addHeader(HttpHeaders.SET_COOKIE, ResponseCookie.from(name, value).build().toString());

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Apr 14, 2020

An easy way around this problem is to use org.springframework.http.ResponseCookie class. The class has a nice builder with samesite property.
Then you can add the cookie to the response header:
response.addHeader(HttpHeaders.SET_COOKIE, ResponseCookie.from(name, value).build().toString());

That's what WebFlux does for Servlet containers and for Netty. For Undertow only it relies on the built-in support.

It is more challenging to do this for Spring MVC where we are not in control of the abstraction. Probably the best route is (for applications) to rely on whatever the underlying server provides since all applications on that server are facing the same issue.

@mdeinum
Copy link
Contributor

mdeinum commented Apr 15, 2020

I did a little investigation and it appears that at all of the out-of-the-box supported containers (Tomcat, Jetty, Netty and Undertow) each have a custom way of enabling and setting the SameSite attribute. Couldn't this be part of the configuration of the embedded containers, that when set to LAX, STRICT it will be enabled with that value? Or at least with the Servlet containers (as it appears to be workable from a reactive point already).

@bclozel bclozel changed the title Allow configuration of session cookie SameSite attribute as an application property Configure SameSite attribute on session Cookies with Spring Session Apr 15, 2020
@bclozel
Copy link
Member

bclozel commented Apr 15, 2020

Since the initial scope of this issue was about Spring Session, and since we can't obviously solve all the different aspects of this support, I've created different issues for Spring WebFlux #20970 and Servlet-based apps #20971. I'll try and add the relevant information in each issue.

In the meantime, I'm closing this issue in favor of #20961 which is addressing the issue without conflating Spring Session support with future Servlet spec support.

@bclozel bclozel closed this as completed Apr 15, 2020
@bclozel bclozel added status: superseded An issue that has been superseded by another and removed type: enhancement A general enhancement labels Apr 15, 2020
@bclozel bclozel removed this from the 2.2.x milestone Apr 15, 2020
@bclozel
Copy link
Member

bclozel commented Apr 15, 2020

@mdeinum I've tried to summarize my findings in #20971. As @rstoyanchev pointed out, I'm not sure we're in a position to provide a consistent support with all Servlet containers. Don't hesitate to comment on that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests