envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.9k stars 4.79k forks source link

Rewritten Path and Domain in Set-Cookie headers should be corrected by Envoy #15612

Open sunjayBhatia opened 3 years ago

sunjayBhatia commented 3 years ago

Description: Using host or path rewrite rules in conjunction with Cookies with Domain or Path attributes can result in what can be considered incorrect behavior.

Given a route config that matches the prefix /foo to /bar an upstream server will likely return a Set-Cookie header with the Path attribute set to /bar<rest-of-match>. However this may be an internal-only path, an implementation detail, etc. so the browser/client etc. then sending that cookie will not send it for requests on the path /foo but instead /bar. This is a similar issue for rewriting the Host/:authority header, the Set-Cookie header may return the rewritten host in the Domain attribute.

It would be useful to have (maybe as an optional configuration) Envoy reverse this for Set-Cookie headers automatically, e.g. for each Set-Cookie header returned by an upstream, undo the rewritten Path or Domain elements to what the original request specified. This could be a RouteAction or maybe evenRouteConfiguration level option. Note this is not a request for generic HTTP header rewriting functionality, though the implementation of such a feature might be made easier by implementing what this issue asks for.

Meta Question It seems this sort of thing is already somewhat possible in Envoy with Lua filters, e.g. have an on-request function in Lua that captures the original path/:authority header before it is rewritten in the router and save it in dynamic metadata and then an on-response function that uses that saved information to reverse the rewritten content (though would have to be configured at the filter chain level and not route level so might have some drawbacks). From searching through Envoy issues it seems this kind of problem is generally responded to with a suggestion to use Lua by the community. Is there a codified stance the Envoy community has taken regarding this and when a feature will not be considered to add to Envoy? Or is it a matter of people getting things to work with Lua and not pursuing further changes to Envoy?

sunjayBhatia commented 3 years ago

Note I (or some other resources at my company) am willing to work on this if it is deemed an acceptable change by the community

wrowe commented 3 years ago

I've added cors because of the implications of wrong domain headers, and http since I didn't see another reverse-proxy topic that seemed more focused. Design proposal would be helpful.

Since the lua solution is solved with appropriate examples, but is more heavy-weight than required 99% of the time, could you tune this ticket to non-lua, baked-in solutions for envoy? A different "Better examples for set-cookie rewriting in lua" would solve the different ask in this ticket.

mattklein123 commented 3 years ago

cc @alyssawilk for thoughts on this.

alyssawilk commented 3 years ago

I think in-house with similar features we usually expect the upstream to be rewrite-aware, but I think it'd be nice to have it as a configurable feature in Envoy. I think it's going to be a tricky one to not miss corner cases for though. @sunjayBhatia if you're up for picking it up please feel free to /assign it my way. cc @antoniovicente as well.

sunjayBhatia commented 3 years ago

I should be able to get started on this next week, I'll write up a little design here and get some feedback before coding it up fully

jnehlmeier commented 3 years ago

After searching an endless amount to time in the envoy docs on how to rewrite cookie path/domain I stumbled upon this issue. Coming from NGINX I could not believe that something like proxy_cookie_domain and proxy_cookie_path is not easily supported by envoy.

Even though it would be nice if envoy will rewrite cookie path/domain automatically based on the original downstream request, it should also be possible to do manual rewrites as well. This would also be analogous to NGINX which allows you to use variables (e.g. $host) or literals in proxy_cookie_domain/path.

For example it should be possible that a downstream request to http://sub.domain.com/path/ will receive cookies for Domain=domain.com; Path=/. With a pure automatically approach this would not be possible.

sunjayBhatia commented 3 years ago

Looking with a bit more detail about how this would work, I'm on board with @jnehlmeier's suggestion about Envoy not trying to figure out the correct path/domain attributes to rewrite a cookie with and requiring an explicit configuration by users how they want their Set-Cookie header to look for a couple reasons:

The cons of this approach are that it may not actually be that hard to do automatic rewriting, and for most cases, it will be enough and we could offer both to cover all use cases. Saving off the original :authority and :path headers and plopping them back into a header seems maybe doable.

jnehlmeier commented 3 years ago
  • We can offer more advanced features in the future if we choose to expose some variables as part of rewrites

Not sure which variables you have in mind but variables for domain and path are required from the beginning when cookie rewrites are configured manually by the user. Otherwise cookie rewrites are not possible for virtual hosts with multiple domains and/or domain wildcards and also not possible for prefix and regex based path rewrites / redirects.

sunjayBhatia commented 3 years ago
  • We can offer more advanced features in the future if we choose to expose some variables as part of rewrites

Not sure which variables you have in mind but variables for domain and path are required from the beginning when cookie rewrites are configured manually by the user. Otherwise cookie rewrites are not possible for virtual hosts with multiple domains and/or domain wildcards and also not possible for prefix and regex based path rewrites / redirects.

Yeah, was mostly thinking about rolling things out incrementally and possibly not covering wildcard domains etc. initially but yes, to cover those sorts of use cases you are correct, a user configuring routing themselves would need access to original host/path variables