haskell-servant / servant

Servat is a Haskell DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.81k stars 409 forks source link

Auth adds Set-Cookie headers to every response #1527

Open MarceloZabini opened 2 years ago

MarceloZabini commented 2 years ago

Hi to all!

It seems to me that any protected endpoint will add both the XSRF protection and Session cookies to the response. Is there a reason why this is necessary?

I ask that because at our shop we have an end-to-end test failing in an endpoint that changes the auth cookie of a logged in user, a failure caused due to the response containing two Set-Cookie: JWT-Cookie=... headers. Apparently it is not deterministic which cookie the browser should pick, so this doesn't always work as intended. Note that this might be hard to replicate in most use cases; With Chromium, we've seen it happen very often only when two requests hit the endpoint within the same second.

But even in a less contrived scenario, I think this could make clearSession fallible. RFC 6265 says

" Although cookies are serialized linearly in the Cookie header, servers SHOULD NOT rely upon the serialization order. In particular, if the Cookie header contains two cookies with the same name (e.g., that were set with different Path or Domain attributes), servers SHOULD NOT rely upon the order in which these cookies appear in the header."

It seems to me the spec above refers to how servers handle cookies, and I couldn't find anything clearly specifying client behaviour, but I guess it might be better avoiding two cookies with the same name in the response nonetheless?

IIUC this could all be addressed by removing Set-Cookie headers from every response, adding them only in acceptLogin, clearSession and similar functions.

Please let me know if you want more details or if I'm just getting things wrong, and thanks!

gdeest commented 2 years ago

~The cookie name is overridable (sessionCookieName in CookieSettings), JWT-Cookie just being the default value:~

https://hackage.haskell.org/package/servant-auth-server-0.4.7.0/docs/Servant-Auth-Server.html#t:CookieSettings

~Would that solve your issue ?~

I am guessing it wouldn't. I'll need to investigate a bit more, but I am not seeing an easy way out.

Could a middleware that removes the duplicate Set-Cookie header perhaps provide a temporary solution to your failing test issue ?

MarceloZabini commented 2 years ago

Could a middleware that removes the duplicate Set-Cookie header perhaps provide a temporary solution to your failing test issue ?

For now, we've dealt with the test by sleeping a few seconds in the right places, which makes the browser pick up the right cookies most of the time for some reason. Thanks for the suggestion, though!

and-pete commented 2 years ago

IIUC this could all be addressed by removing Set-Cookie headers from every response, adding them only in acceptLogin, clearSession and similar functions.

Please let me know if you want more details or if I'm just getting things wrong, and thanks!

Hello Marcelo!

Is it possible for you to show what your API type's structure is?

The reason I ask is that each time I've seen someone report double cases of a JWT-Cookie it has been something to do with the way that their API has been structured For example: putting the either/both the login route and/or the clearSession route that returns content wrapped in Headers '[Header "Set-Cookie" SetCookie, Header "Set-Cookie" SetCookie] in a endpoint protected by the Auth combinator itself, instead of in an unprotected route.

As for why the Auth combinator returns a new JWT-containing session cookie with each response to a valid request to one of its children..

The new Set-Cookie that comes with every response protected by the Auth combinator is there, for one, so that a user's "session" is extended if they are actively hitting those protected endpoints. That is, each new Set-Cookie comes with the same cookieMaxAge setting — if that setting has been set in the creation of CookieSettings — that the browser will then apply to the current time upon receiving it to set its expiry date. My opinion is that this automatic refresh of the token&session probably suits the smaller scope of this library (as it stands now, anyway) more than say having a separate refresh token endpoint. But from the library's history that I've gleaned it's all very much an intentional choice.

Sidetrack for posterity... If anyone reading is trying to decide between using the cookieMaxAge setting vs using the cookieExpires setting in the creation of your initial CookieSettings, you should go with cookieMaxAge. The CookieSettings referred to by the code underpinning the Auth combinator (e.g. for creating each new Set-Cookie upon a valid request to one of its protected routes) is the immutable value that was passed into the Servant Context prior to server initialization.

I've seen several codebases using servant-auth-server where the developer set cookieExpires in their login handler based on the current time + some duration, not realizing that initial JWT-containing session Set-Cookie would be overwritten by the new Set-Cookie received with the first response to one of the protected routes (or lead to a second cookie with the JWT-Cookie name, depending on what they're modifying. I'd need to dive into RFC 6265 to check the exact rules again).

I ask that because at our shop we have an end-to-end test failing in an endpoint that changes the auth cookie of a logged in user

This sounds like it might be a similar thing in your case.

Inherited opinion territory, but if we're at the point where we're modifying the session cookie, I'm lead to understand we might be better off making our own implementation using something like Servant.Server.Experimental.Auth and Servant.API.Experimental.Auth. This lets you pass in your own custom handler into the Context, so that you control exactly what is going on in the routes protected by its own combinator: AuthProtect and you aren't constrained by servant-auth-server's narrow happy path.

Anyway! Curious to see your API type (if it's possible to share a version of it) to see if I can spot anything or if I'm way off base here :)

MarceloZabini commented 2 years ago

For example: putting the either/both the login route and/or the clearSession route that returns content wrapped in Headers '[Header "Set-Cookie" SetCookie, Header "Set-Cookie" SetCookie] in a endpoint protected by the Auth combinator itself, instead of in an unprotected route.

That's definitely the case for this particular API endpoint that leads to test failures, although it is not for our "login" endpoint. What happens is we store an extra attribute in the cookie that can be changed - but only by authenticated users - which you can think of as a role, and we do that by calling acceptLogin + applyCookies again simply because this was easy enough to implement at the time and seemed to work during manual testing.

Also, thanks for the explanation on cookieMaxAge and cookieExpires. We've been using cookieExpires, without renewing it in the request handler like you mentioned (so I guess we're doing the right thing there).

So IIUC sending auth cookie headers in every Auth-protected response is because by default servant implements a sliding expiration window. I'm not an experienced web developer so it may just be these are standard things I'm not aware of, but I have a question about that behaviour.

Does it make sense to send these cookies when setting cookieExpires instead of cookieMaxAge? They'll be the same expiry dates every time so it's effectively not a sliding expiration window. Also, the difference in behaviour depending on how settings are configured is quite surprising to me. I couldn't find this documented anywhere (but I haven't looked too deeply). Is it perhaps worth documenting the sliding expiration window when using cookieMaxAge? But also, is it possible/desirable to change this behaviour when using cookieExpires?

Inherited opinion territory, but if we're at the point where we're modifying the session cookie, I'm lead to understand we might be better off making our own implementation using something like Servant.Server.Experimental.Auth and Servant.API.Experimental.Auth.

Thanks for the pointers! We're deciding whether to use a separate cookie or a custom header for this attribute we currently have in the session cookie, but it'll be valuable to look at these experimental features to guide that decision.

Is it possible for you to show what your API type's structure is?

It feels like I've given the details of our API you probably wanted, but let me know if you need more details otherwise!

qwbarch commented 1 year ago

I'm a bit confused. Doesn't servant-auth-server make use of cookieExpires, rather than cookieMaxAge? https://github.com/haskell-servant/servant/blob/2323906080e50fc2774cd1b43bc59548a90152ed/servant-auth/servant-auth-server/src/Servant/Auth/Server/Internal/Cookie.hs#L80-L90

I've tried to set cookieMaxAge to 1 second, but the JWT is still able to be used after a second of it existing.
I then tried to set cookieExpires to the current time of when the server was created, but this also doesn't expire after a second.

How exactly do I go about expiring my generated JWTs? I don't want to give a malicious user a token with an infinite lifetime. In regards to @and-pete's comment, if I understand your explanation right, does the JWT get "refreshed" by itself? I tried decoding a couple generated JWTs and put them into jwt.io, and discovered there was no expiry date set. Is that intended behaviour?

qwbarch commented 1 year ago

For whatever reason I am able to get the JWT to expire now.
If I understand this correctly, servant-auth-server's default is to have a non-expiring JWT, and handle its "life-time" through the cookie expiring. On each subsequent request, the cookie containing the JWT's expiry is extended.

While this does handle expiry on the client side, does this not allow malicious users to keep using that same JWT for as long as they want, until the signing key used for the tokens is changed?

and-pete commented 1 year ago

does this not allow malicious users to keep using that same JWT for as long as they want, until the signing key used for the tokens is changed?

Correct. Without cookieExpires (and therefore the exp claim) set, the JWT contained within the session cookie could be continually used until signing keys change or a blacklisting system is setup (with this latter case seeming to run somewhat counter to the stateless philosophy behind JWTs).

So what servant-auth-server does is use that cookieExpires field in two places:

  1. As you identified in makeJWT in the creation of the encoded token itself (i.e. the exp claim again in the JSON); and
  2. In setting the Expires field of the set-cookie containing the above token

In 2. just then I'm referring to the call to applyCookieSettings on line 89 of your snippet (or here), which in turn calls cookieExpires further down on line 99 (here).

...where, if the Expries attribute is absent on the set-cookie, a user's browser will toss away the cookie at the end of the current session. See MDN's page for the Set-Cookie attributes here for how Expires vs Max-Age (and their interaction) should be treated by browsers.

But there's obviously no corresponding rule that says a JWT is invalid from the server's perspective if it is missing an exp claim (only if it is present but expired does it matter).

So that one cookieExpires setting has two different purposes in servant-auth-server.

  1. The expiry value is included on the set-cookie itself, which is mostly for the browser to help manage things on its end (if we're looking at a webapp, anyway). It's not like if a client sent back a cookie with their request and included the expiry field from our prior set-cookie that we could even trust it on the server end.
  2. And the expiry value is encoded in the JSON structure of the JWT itself, which is contained within that cookie. And that we, the server who signed it and presumably set that exp claim in the first place (....or we can at least verify the keys of the server that did), can trust.

And the default value of cookieExpires in defaultCookieSettings :: CookieSettings is indeed Nothing.

I'm inclined toward thinking they should have their own separate configuration fields anyway, at the least.

And if I set cookieExpires in CookieSettings for the UTCTime of around Nov 1st 2022 when I compile and then start my server this time, do I then want to have to recompile and restart that server every few weeks so that it's issuing new tokens with a later expiry time in its exp field? No. What if I want to have uptime from now going past Nov 1st 2022/etc., but also don't want my tokens to never expire? (or be something long like annual or something? which may be okay if you're fine with setting a hard limit for when you'll need to recompile and relaunch your server by)

Because the CookieSettings (and therefore cookieExpires :: Maybe UTCTime value) that you start your start your server with is immutable currently, you're a bit out of luck. As that immutable value is what all of the endpoints protected by your Auth combinator are going to be referring to when creating the new JWTs to send back to you upon each valid request from you (that included a valid JWT).

As you search around GitHub for examples, you'll find some examples of people abusing the acceptLogin function in their protected endpoints in an attempt to actually send back JWTs with refreshed expiry times in the response to each valid request (as you can pass in a CookieSettings that HAS been freshly-updated with a new expiry to that acceptLogin function...).

But.... eugh. Their clients then get 2 sets of tokens from the protected endpoints. The one the Auth combinator provides based on the CookieSettings stashed in the Servant Context (see here). And the second token-containing headers created by the acceptLogin-function abuse with an updated expiry.

Anyway... 😅

qwbarch commented 1 year ago

The design of servant-auth-server strikes me a bit weird even after your explanation.

Because the CookieSettings (and therefore cookieExpires :: Maybe UTCTime value) that you start your start your server with is immutable currently, you're a bit out of luck. As that immutable value is what all of the endpoints protected by your Auth combinator are going to be referring to when creating the new JWTs to send back to you upon each valid request from you (that included a valid JWT).

This is what's been confusing me as well, typically I'd set my tokens' exp claim to be around 15 minutes so it's as short-lived as possible.

As you mentioned, using acceptLogin with a different expiry is not an option due to the Auth combinator creating its own token.
My server doesn't have the luxury of restarting every couple weeks to invalidate tokens, so I'll probably have to end up dropping this library for now unfortunately.

I really do appreciate the detailed response you've given me, I was reading the old repository's issues before as well and found many of your other replies there too. Thank you!

and-pete commented 1 year ago

I really do appreciate the detailed response you've given me, I was reading the old repository's issues before as well and found many of your other replies there too. Thank you!

No problem. I figure some other people should benefit from the silly amount of time I spent trying to work out what's going on with these config values.

If I ran my own fork for working around this, I feel like the simplest patch would be to drive everything from the cookieMaxAge config value. i.e. fetch the current UTCTime as the first line in makeSessionCookie, then apply the maximum age from CookieSettings to that value to get a desired expiry time, and pass in that expiry time as the Just expiry to makeJWT.