sync_gateway icon indicating copy to clipboard operation
sync_gateway copied to clipboard

Adds the ability to set the domain at which Sync Gateway's session cookie is set

Open mz2 opened this issue 6 years ago • 10 comments

Adds the ability to customize the domain at which SG sets its cookies, largely to avoid issues with making session cookie authorized Sync Gateway calls from cross-origin requests made by a browser based client.

With these changes, the cookie domain can be optionally set per incoming request origin, allowing for SG set cookies to work in at least the following scenarios:

  1. browser based frontend application at subdomain spa.x of domain x wanting to receive and post back cookies set by Sync Gateway at sg.x. In our case Sync Gateway's cookie is set by an API server, let's call it api.x, which in turn makes API calls with sg.x -- api.x cannot set a cookie for sg.x (which is SG's effective behaviour presently at session extension time because the Domain property is not set in the cookie => browser's default behaviour is to treat the domain as the domain for where the response came from) but it can for x. This change makes it possible for the cookie to behave sensibly at session expiry extension time (it becomes possible to configure SG to respond with Domain=x, effectively extending the already set cookie for x instead of setting a new cookie for sg.x).
  2. SG exposed as sg.x and sg.y for two different browser based client applications at domains x and y.

mz2 avatar Nov 07 '18 16:11 mz2

Any thoughts on this @adamcfraser or others? I know this PR came out of nowhere without an issue preceding it, but it's really a pretty critical bugfix category issue in our project's use of SG due to the point 1 raised above, and alternatives involve putting some otherwise unnecessary reverse proxying in place to avoid the issue.

mz2 avatar Nov 12 '18 12:11 mz2

@mz2 I've taken an initial look at this, but haven't yet been able to fully assess any potential security holes that this would open up.

Can you help me understand a few details:

  1. Can you provide more specifics on your auth flow today, and specifically where the current flow is breaking down without this enhancement?
  2. Is this something that could be done by your auth server (api.x in your example above) - i.e. rewriting the cookies as needed based on the token returned when creating the session?
  3. Would any of this be alleviated using the existing CORS support?

Thanks.

adamcfraser avatar Nov 14 '18 18:11 adamcfraser

Is this something that could be done by your auth server (api.x in your example above) - i.e. rewriting the cookies as needed based on the token returned when creating the session?

We do indeed do that: the auth server (let's continue to call it "api.x"). When the API server communicates with Sync Gateway to create a session for user, the domain property of the cookie that is set for domain "x". We set it on "x" because the auth server at hostname "api.x" cannot set a cookie for "sg.x" (browsers would do not store such a cookie).

The problem with Sync Gateway in this setup before these changes is that when the session reaches 10% of time from the expiry time, it itself sets a cookie in its response (in order to express extending the session), and because the domain property of the cookie that is set by Sync Gateway is not set, browsers will store it as a cookie for the domain sg.x. This is a problem: now we have the browser sending cookie for "x" and "sg.x", only the one for "x" no longer being really valid, and being picked up by SG presumably because it is more "general").

Can you provide more specifics on your auth flow today, and specifically where the current flow is breaking down without this enhancement?

We do precisely what you proposed we do above, i.e. we authenticate the user with api.x, and rewrite the cookie domain. As noted above, this works fine… for the 10% of the expiry time of the cookie.

  1. User logs in at https://dev.manuscripts.io with the web browser (or native Mac / iOS app that similarly uses a browser web view and is "CORS aware" as its resources are served from a specific special URL scheme that the app has a handler for – a practical requirement for using WKWebView in the process).
  2. On successful authentication challenge, a session for the user matching the user's Sync Gateway user record is returned by the login response to api.x, with the cookie set to "x" (api.x cannot set the cookie for "sg.x").
  3. Web browser / native app's web view then makes requests to Sync Gateway, including the cookies it stored from the above process.
  4. Wait for 10% of the expiry time to pass, and make a request to Sync Gateway, and observe 💩💥.

Would any of this be alleviated using the existing CORS support?

No – the issue with current CORS related configurability of Sync Gateway (i.e. the set of allowed origins) with the session cookies is that all the resources, client and server included, need to be at different subdomains of "x" in order for the cookies to be stored and sent by the browser back to the server.

  • Sync Gateway is at hostname "sg.x" and (before these changes) always sets a cookie for its full hostname (as the domain is not specified).
  • "api.x." sets a cookie on "x" (it cannot set a cookie at "sg.x" as browsers would not store it).
  • Web browser frontend served from origin "https://www.x" stores and sends in its requests to "x" cookies for domain "x", as well as the unwanted "sg.x" (which could with these changes instead be allowed to replace the cookie made available for "x").

The related problem that also applies to our system before these changes is that we also have an application in the making at domain "y" where we would like to use the same Sync Gateway instance. Whilst the CORS configuration allows for "https://www.y" to be amongst acceptable origins for the request, again the cookie that Sync Gateway sets not including the domain property will mess with things. I could not think of another way to prevent the 💩 💥 that again would happen past the 10% from expiry point… without matching the origins to the wanted domain values.

mz2 avatar Nov 17 '18 15:11 mz2

@mz2 Thanks for the details - I agree this is a legitimate issue, particularly around the way SG issues a SetCookie to extend session lifespan. I can see how forcing the domain on the cookie avoids this, but want to confirm that this is actually the preferred solution.

I want to make sure I understand exactly what's failing once the 10% expiry has elapsed. As I understand, the scenario is something like the following:

  • Initial cookie has domain:x, expiry set (say 60s)
  • On authentication after 10% time, SG returns SetCookie without domain set, that has updated expiry
  • This results in two cookies stored on the client - one for x (non-refreshed expiry) and one for sg.x (refreshed expiry)
  • Subsequent requests to Sync Gateway send both cookies (as both apply based on domain, and neither have expired yet). Sync Gateway currently just uses the first cookie it finds matching the target cookie name, and so could end up grabbing either one.

At this point, I don't follow the details on what's failing. Although redundant cookies are being sent, both represent a valid session. The session id will be unchanged across extended expiration, so it's not the case that the original cookie is no longer valid. It's true the original domain=x session will expiry early, but that would just result in clients to be left with the sg.x cookie.

Can you let me know what I'm missing in this scenario?

adamcfraser avatar Nov 23 '18 20:11 adamcfraser

@adamcfraser oh yes, that was hazy: the issue on that cookie extension topic is at least what happens at logout time: if user logs out within the expiry time (i.e. with default settings within 24h). As with login, this action needs to be in our application (and I would guess in many others) completed by a separate API server which amongst other resources can clear the Sync Gateway session cookie (which as expained above needs to be set at domain x in this topology)… and in response clears its cookie. Before these changes, if the session was extended, a browser client can be left with a stale cookie for sg.x that it cannot clear, which is Bad (user is left unknowingly in a logged in state, and a subsequent login will either work or not work depending on when the next login is made – was a rather fun issue for my team to find!)

The other reason for this proposed change is arguably a new feature, arguably a bug fix: make the cookie setting behaviour compatible with Sync Gateway being served at multiple hostnames.

mz2 avatar Nov 23 '18 21:11 mz2

@mz2 If your logout API actively deletes the Sync Gateway session (i.e. issues a DELETE /_session/sessionid against the admin API), then you wouldn't have any cases of unexpected subsequent successful logins. An attempt to delete the session via the public API (i.e. using one of the valid session tokens to issue DELETE /_session against the public API) also invalidates the underlying session.

I agree that either one of these approaches is going to leave at least one stale cookie on the browser client in scenarios where the client has previous extended the expiry and ended up with x and sg.x versions of the cookie. Assuming the session has been explicitly deleted by a logout, though, neither of these should permit login. If you're seeing clients subsequently able to login, that might indicate an a more serious issue (i.e. SG session invalidation shouldn't depend on clients not reusing stale cookies). Can you provide any more details on the behaviour you were seeing?

adamcfraser avatar Nov 23 '18 22:11 adamcfraser

Duh, claim of being left in a logged in state was just it being late. Indeed we delete the session, the cookie wouldn't actually work, that's true. The issue is actually the opposite, it can end up stale and given which one of cookies that have been set is undefined, 401s can result.

On top of this, there is also the use of using the same SG instance from multiple hostnames without complicating reverse proxying.

mz2 avatar Nov 23 '18 22:11 mz2

@mz2 Ah - that makes sense. On a logout/login situation, you could end up with the cookie for domain x being valid, but a stale session cookie for sg.x.

This could be addressed by Sync Gateway evaluating all matching cookies being provided with the request (instead of only checking the first matching cookie). That feels like a less intrusive change - in particular it doesn't introduce new config elements that need to be maintained going forward.

However, I agree that the new SetCookie issued by Sync Gateway during automatic session extension will break attempts to set up CORS for the multiple hostname scenario. A few clarifying questions on that scenario:

  • Would an option to simply disable auto-extension of session lifespan be a feasible solution for your use case?
  • Is it strictly necessary to define a mapping from origin to cookie domains? (SessionCookieDomains) What breaks if it's just a single domain (i.e. SG's domain) that's included on all cookies?

adamcfraser avatar Nov 23 '18 22:11 adamcfraser

Would an option to simply disable auto-extension of session lifespan be a feasible solution for your use case?

I'd argue this to be a workaround: any web browser client where the client gets served by some server unrelated to the Sync Gateway would have the same issue (the cookie won't be set for the same subdomain). Extending the cookie period is a pretty useful feature.

Is it strictly necessary to define a mapping from origin to cookie domains? (SessionCookieDomains) What breaks if it's just a single domain (i.e. SG's domain) that's included on all cookies?

What breaks in our case is the ability to build two different products that share the same data via a shared Sync Gateway connected bucket (two different products that integrate with each other effectively through Sync Gateway). Any alternatives I can think of involve complicating the reverse proxying setup.

mz2 avatar Nov 26 '18 10:11 mz2

This could be addressed by Sync Gateway evaluating all matching cookies being provided with the request (instead of only checking the first matching cookie).

This seems messy: you'd end up still in a situation where the client in a common case holds two copies of the cookie, with the server(s) being able to unset only one of them.

mz2 avatar Nov 26 '18 11:11 mz2

Closing a 5 year old PR that is unmergeable.

torcolvin avatar Jan 25 '23 13:01 torcolvin