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

Bad Request due to state cookie are piling up and sending to server #331

Closed
rithoria opened this issue Feb 21, 2018 · 27 comments
Closed

Bad Request due to state cookie are piling up and sending to server #331

rithoria opened this issue Feb 21, 2018 · 27 comments

Comments

@rithoria
Copy link

Environment
  • mod_auth_openidc version (2.3.3.)
  • Apache version (2.4)
  • platform/distro (Linux)
Expected behavior

Server should only receive latest valid state cookie. Not those which are just there and completed authentication.

Actual behavior

Approach to protected path without several times (7-8) times without closing browser tab. Then authenticate.
redirect uri which is /callback in our case throw error 400 Bad Request.

@rithoria
Copy link
Author

Is there any way, I can discard all in-valid state cookies on Apache layer and use only the latest valid one. I am able to produce this behavior in Chrome.

@stewartthomasj
Copy link

I think I am seeing this same issue as well. See here:
https://groups.google.com/forum/#!topic/mod_auth_openidc/hRQfHTbbtFY

@zandbelt
Copy link
Member

zandbelt commented Mar 21, 2018

multiple state cookies are required to allow parallel requests; when doing too much of those, the browser DoS-es itself; that is the intended behavior and you should architect your application in such a way (according to the guidelines on the Cookies wiki) so this does not occur

@MattKetmo
Copy link

Hello,

Using the last release we are running the exact same issue where many xhr requests induce many cookies creation after session timeout. The apps behind the proxy are not apps we're developing (eg. Jenkins, Grafana) so there are no way to "architect our application" differently and we don't have an exhaustive list of path to whitelist, which won't be maintainable anyway (and they even don't have the XMLHttpRequest header).

I understand "multiple state cookies are required to allow parallel requests", but in this case can it make sense to have an option (disabled by default) to limit the max amount of cookies generated (by deleting the previous ones or not sending others)? This should be fine in most scenarios.

@zandbelt
Copy link
Member

The proposed solution is not as great as you may initially think: limiting the max amount of cookies effectively means limiting the number of parallel requests. Some day you'll hit that maximum number and you'll be in the same situation as today. There will always be (actually a lot of) edge cases which will lead to a user having to delete cookies manually once entering this "overload" state. The only deterministic alternative is not to allow parallel requests.

Also: currently the module cleans up cookies after the state timeout has occurred. So reducing the state timeout may work for you. Otherwise you may file a (new) issue with the proposed way forward and we can discuss that.

@MattKetmo
Copy link

I know this is not an "ideal" solution but it would be "acceptable". Setting a reasonable limit like 10 will be more than enough for the users to not hit the limit in 99% the times.

We already tried to reduce the state timeout but some tools like Grafana can sends dozens of xhr requests in few seconds when dashboard contains lots of graph. So not an acceptable solution either.
And asking users to delete theirs cookies can be a huge pain for non-tech persons.

As suggested I'm going to open a new issue to discuss about it 👍

@LaurentDumont
Copy link

@MattKetmo Just wanted to chime-in that we've also been hitting the same issue with Grafana with the most "busy" dashboards.

Chrome ends with a dozen or more mod_auth_openidc_state cookies and we have to clear the cookies manually which is not really friendly. Strangely enough, we've been getting reports of Firefox not seeing the issue as often.

@zandbelt
Copy link
Member

zandbelt commented Aug 1, 2018

I would like to stick with the #1 advice to re-architect the app (you'll find out why), but if that's not possible we can search for workarounds. Let's say we limits the number of outstanding state cookies (i.e. parallel requests):

this leads to a situation where requests are being refused because there are a bunch of outstanding requests already. Note that mod_auth_openidc cleans up state cookies for requests that have never been completed, but of course that's done only after they expire, say after 5 minutes. Now the question becomes what's next:

the user then has to wait until the first of those state cookies has timed out, perhaps 5 minutes, hopefully beating the XHR requests to it that are most likely still being fired in parallel.

Is that what you're looking for or do you have a better suggestion?

@zandbelt
Copy link
Member

zandbelt commented Aug 1, 2018

Also: is there perhaps a way to detect from the request whether it is a XHR request, other than the unfortunatelt non-supported X-Requested-With header? Because that would be the most preferred, optimal solution... even if it is Grafana-specific we may be able to generalize the option somewhat.

@zandbelt zandbelt reopened this Aug 1, 2018
zandbelt added a commit that referenced this issue Aug 1, 2018
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
@zandbelt
Copy link
Member

zandbelt commented Aug 1, 2018

Can you comment on this branch: https://github.com/zmartzone/mod_auth_openidc/tree/limit-max-number-of-state-cookies
It is completely untested for now, but I'm curious about your feedback (in addition to the feedback on the questions above)

@zandbelt
Copy link
Member

zandbelt commented Aug 2, 2018

I've tested it now and I must admit I like it more that I anticipated because even though things can (and actually do) go wrong in the same way, after a while mod_auth_openidc will now automatically recover instead of depending on the user having to take some manual action like removing cookies or restarting the browser. If you agree and you have no further refinements I will consider making this the default with e.g. a setting of max 5 parallel requests.

@zandbelt
Copy link
Member

zandbelt commented Aug 3, 2018

That is merged to the master branch now, with the default max number set to 7.

However, there hasn't been an awful lot of feedback: as it stands now, when hitting the limit, an error HTML page is produced with a HTTP 503 Service Unavailable. That's nice in case users are trying to access a page in their browser. I'd still like to see what people think when using this with XHR applications: is it also workable in that case to receive a 503 and hope that the XHR request rate backs off a bit (at least the HTML is useless then)?

@MattKetmo
Copy link

Hello Hans, thank you for this workaround. I'm not sure the 5xx will rate limit the number of future requests but that's way better to have this option now.

So if I understand correctly, instead of having a 400, the user will face a 503 when the limit is reached, and will have to wait for the cookies expiration instead of manual cleaning. That of course not perfect but reasonable.

I have a naive question: if the server still return the 301 redirection (instead of the 503) but without providing an new cookie, the older cookies can't be used to validate the state, right?

@zandbelt
Copy link
Member

zandbelt commented Aug 3, 2018

The redirect is effectively what OIDC calls an authentication request: it is required that the authentication request contains a "state" parameter that is cryptographically bound to the browser - see the OAuth 2.0 spec https://tools.ietf.org/html/rfc6749#section-10.12 - which in its turn means that it is bound via/in/with/using a cookie. Since the "state" parameter must be unique - again by spec - for each authentication request, this means that a new cookie must be generated for each of those.

@zandbelt
Copy link
Member

zandbelt commented Aug 3, 2018

FYI: binary packages for 2.3.8rc2 which include this feature are now available on: http://mod-auth-openidc.org/download/?C=M;O=D

zandbelt added a commit that referenced this issue Aug 4, 2018
since it turns the HTTP 503 status code into a 200 which we don't prefer
for XHR clients; users will see Apache specific readable text

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
@zandbelt
Copy link
Member

To conclude this I'd still like to know:

  • is there a way to detect from the request whether it is a XHR request, other than the unfortunately non-supported X-Requested-With header? (even if it is Grafana-specific we may be able to generalize the option)

  • is the stock 503 Apache error message sufficient for both browsers and XHR clients?

  • has anyone tried this with an application that suffers from the state cookie problem i.e. Grafana; if so, how is the user experience?

@MattKetmo
Copy link

  • There is no way to detect an XHR request when the X-Requested-With header is not set. However the Accept header is usually different between main doc request and additional requests (text/html ... vs application/json or other mime type or even not set at all). That's in my opinion the best way to differentiate those requests.

  • 503 errors are not really sufficient and does not provide a much better ux than the 400 with too many cookies. Best way still is to relaunch the browser (or open developer console) when it happens. We limited a little bit this situation by increasing our session validity + injecting a Refresh http header before it expires. But that's very hacky.

zandbelt added a commit that referenced this issue Aug 22, 2018
version 2.3.8rc5

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
@zandbelt
Copy link
Member

I've improved auto-detection of XMLHttpRequests in 2.3.8rc5 which you can download from: http://mod-auth-openidc.org/download/?C=M;O=D

It takes into account the Accept: header now though I guess most XMLHttpRequests would use */* anyway out of the box so it would require a well-behaved client to leverage this.

Feedback is welcome.

@zandbelt
Copy link
Member

This is out in 2.3.8 now.

@hnordberg
Copy link

This may be a silly question or perhaps has been answered before, but why not set the cookies to expire so that the user agent cleans them up?

@zandbelt
Copy link
Member

the point is that the cookies need to live for a certain time (as they represent an outstanding request that may be completed with a response) and they're piling up so fast within that time window that things break; anyway, 2.3.8 will limit the max number of outstanding requests/cookies by 7

@hnordberg
Copy link

Yes, I get that that is an issue. Another effect of this is that once the user has reached the # cookie / header size limit if they come back the next day they would be able to login if the cookies had expired (since the UA would not have sent the expired cookies). As it is now we have users with 200 cookies and our servers returning HTTP 400 day after day.

Having a reasonable timeout would alleviate this issue.

@zandbelt
Copy link
Member

zandbelt commented Sep 25, 2018

The state cookies are session cookies; a browser-based expiry timestamp would make then persistent which is even worse IMHO. Anyway, server-side cleanup with a max of 7 is the default behavior since 2.3.8, please use that.

@noeldieschburg
Copy link

Hello, If I understand well, when a user has reached the limit number of cookies, he has to restart his browser to clean them out as they are session cookie. What exactly is server cleaning? Is there a way for a user to gain back access to the application without restarting the browser?

@zandbelt
Copy link
Member

zandbelt commented Oct 4, 2018

Yes, wait for the OIDCStateTimeout, see: https://github.com/zmartzone/mod_auth_openidc/blob/v2.3.8/auth_openidc.conf#L687 which is 5 minutes by default. That is the time the user should wait for the first request to time out.

@zandbelt
Copy link
Member

zandbelt commented Jan 7, 2021

for completeness, a number of workarounds have been added since 2.3.8, as described here: https://github.com/zmartzone/mod_auth_openidc/wiki/Cookies#state-cookies-are-piling-up

jhrozek pushed a commit to jhrozek/mod_auth_openidc that referenced this issue Oct 18, 2021
…nIDC#331

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 5041e0f)
jhrozek pushed a commit to jhrozek/mod_auth_openidc that referenced this issue Oct 18, 2021
since it turns the HTTP 503 status code into a 200 which we don't prefer
for XHR clients; users will see Apache specific readable text

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 9e98f1a)
jhrozek pushed a commit to jhrozek/mod_auth_openidc that referenced this issue Oct 18, 2021
…IDC#331

version 2.3.8rc5

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit cff35bb)
@zandbelt
Copy link
Member

more "closure" on this: versions >= 2.4.10 have a (significantly) improved auto-detection mechanism for non-redirect-auth capable requests, see: https://github.com/zmartzone/mod_auth_openidc/releases/tag/v2.4.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants