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

CookieWebSessionIdResolver doesn't recognise https scheme when used with ForwardedHeaderFilter [SPR-17072] #21610

Closed
spring-projects-issues opened this issue Jul 20, 2018 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jul 20, 2018

Mahan Hashemizadeh opened SPR-17072 and commented

CookieWebSessionIdResolver sets the secure flag if it detects that the scheme is https.

Note: SPR-16980 adds the ability to configure this yourself.

If ones setup is such that SSL termination occurs at the load balancer then the server will always think that the request is http.

In the Spring Webflux documentation it states that one can use the ForwardedHeaderFilter (https://docs.spring.io/spring/docs/current/spring-framework-reference/web-reactive.html#webflux-filters-forwarded-headers) to address this issue.

However this does not work (appears that the CookieWebSessionIdResolver is evaluated before the forwarded header filter has had a chance to mutate the uri).

I have created a github repository to reproduce this: https://github.com/mahanhz/secureflag

To check that my custom forwarded header filter works I compared the session cookie secure flag to the Strict-Transport-Security header in StrictTransportSecurityServerHttpHeadersWriter of spring-security (which sets the header using the same approach).

When the sample application is run using http via gradle:

SPRING_PROFILES_ACTIVE=default,usingHttp ./gradlew bootRun

Then there is no secure flag on the session cookie and there is also no Strict-Transport-Security header which is what I would expect.

However when the sample application is run using https via gradle:

./gradlew bootRun

Then there is still no secure flag on the session cookie but the Strict-Transport-Security header is now present which indicates that CookieWebSessionIdResolver does not recognise that the uri is https but StrictTransportSecurityServerHttpHeadersWriter does.


Issue Links:

Referenced from: commits a8a1fc6, 2e4f5a7, 41aa421

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 23, 2018

Rossen Stoyanchev commented

This is most likely because the WebSession is initialized earlier with a reference to the original, unmodified ServerWebExchange. We might have to have the ForwardedHeaderFilter to also re-initialize the WebSession if the protocol changes.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 23, 2018

Mahan Hashemizadeh commented

I'm attempting to understand the flow. Does this seem accurate:

  1.  In HttpWebHandlerAdapter a new DefaultServerWebExchange is created.
  2. When it gets to the custom ProxyForwardedHeaderFilter it wraps the previously created DefaultServerWebExchange in DefaultServerWebExchangeBuilder MutativeDecorator
  3. Spring Security recognises that the ServerWebExchange is a MutativeDecorator and thus it sees that the scheme is https
  4. DefaultWebSessionManager recognises the ServerWebExchange as the original DefaultServerWebExchange in which case sees the scheme as http

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 23, 2018

Rossen Stoyanchev commented

Yes, but 4) is really part of 1).

The Mono<WebSession> is created in the DefaultServerWebExchange constructor and while the session is not retrieved until later when getSession() is used and subscribed to, the code in DefaultWebSessionManager#retrieve still has the original ServerWebExchange instance.

Note I did not actually confirm this but the issue is evident.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 24, 2018

Rossen Stoyanchev commented

Rob Winch, Vedran Pavic, I looked into the idea of re-initializing the WebSession when the ServerWebExchange is mutated but it does not look like a promising direction. While such a solution would be intended only for the ForwardedHeaderFilter to use, in practice whether it works transparently (e.g. whenever path or protocol change) or explicitly (i.e. on-demand re-initialization of the session), we would now have to account for the fact that at any point a decorated exchange might start using a newly initialized session, while the old one is ignored. Not sure if that's a big concern for you or not but it seems like an extra complication? That aside there is also a technical challenges since the mutation code does not know anything about the ServerWebExchange implementation that in turn knows about the WebSessionStore..

As an alternative.. currently ForwardedHeaderFilter needs a ServerWebExchange as input, while the ServerWebExchange needs forwarded headers applied to initialize the session, which is a cycle but the logic of ForwardedHeaderFilter only changes the request. So forwarded header logic could be applied earlier to the request before it is used to create the exchange. The logic from ForwardedHeaderFilter would be re-packaged as a Function<ServerHttpRequest, ServerHttpRequest>) and enabled via WebFluxConfigurerForwardedHeaderFilter would be deprecated. If declared in the list of filters, it would be removed and request function applied instead.

This is achievable for 5.1 still but I'm postponing to RC2 to allow time for comments.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 3, 2018

Rossen Stoyanchev commented

This change is now in. See comments on commit a8a1fc. Effectively ForwardedHeaderTransformer supercedes `ForwardedHeaderFilter. The commit also includes a documentation update.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 6, 2018

Mahan Hashemizadeh commented

Great. Looking forward to using this in 5.1.

I am a bit curious about why the initial capacity of the LinkedHashSet is set to 5:

static final Set<String> FORWARDED_HEADER_NAMES = new LinkedHashSet<>(5);

Given that there are 6 items in the set and the load factor is 0.75 I would have left it blank (easier to maintain) or set it to 9 (to be on the safe side), but maybe 5 is fine or it's a negligible matter.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 6, 2018

Rossen Stoyanchev commented

Oops I'll fix that. I think the 6th one was added more recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants