Improve the CSRF Failure experience #708
Conversation
|
For #611 We've started chatting about adding a nonce to the CRSF cookie name - so multiple tabs won't make CSRF cookies that clobber each other. I need to think about the security implications more, but I think that is the ideal fix for that one. |
|
@NickMeves ah interesting. What are the security implications of having per-auth-request-csrf tokens as compared to one global csrf token? I'm used the standard web development way of having one csrf token for the whole browsing session, or perhaps even a long-lived one across sessions. The important thing being that the principal can demonstrate that it can do a request, read some information and present it back to the server |
I think the challenge is the CSRF token is needed in the process of creating a session, so we don't have a session for a global CSRF token. So we have a CSRF cookie created that aligns with part of the state field during the login redirects. Just need to figure out the right we to deconflict multiple tabs trying creating these CSRF cookies and squashing each other that doesn't have any security implications (I don't know what they are, I need to think and threat model this more). |
|
I have mixed feelings about this PR, I'm in favour of having the redirect to go back to the app, as a user actionably item, but I think that having the option to skip the error page would cause confusion to users, they think they authed successfully, don't see an error page and then are back on their app, I don't think that's desirable UX. I would rather users always see error pages, but having the option to go back to the original redirect URL, as a manual step, I think will definitely help improve the UX (though if the user isn't logged in? does it actually help?) |
|
@NickMeves @JoelSpeed Thanks for the feedback I agree that the most important part of this PR is to improve GUI, giving them a button to click to resolve the issue. As you know we've seen this issue happening in two cases: either the user has bookmarked the OIDC login page, or the user has multiple tabs open. We think that those two cases could be mostly resolved by automatically redirecting the user to the app (which would either finish successfully or restart the whole oauth flow). But the auto-redirection might be prone to infinite redirect loops and possibly security issues? (Although I made sure redirection happens after Redirection Url validation). Would you prefer a PR that only deals with the GUI updates for now? |
|
My main concern with the redirects is an infinite loop, which I'd like to avoid. For now, can we update this PR to just be the GUI updates and discuss the redirection as a separate issue? |
|
Updated the PR to only include the GUI updates. Do you guys know if the go template escaping is enough to deal with putting the URL in the like this? |
jonerer
force-pushed
the
csrf_failure
branch
2 times, most recently
from
383e6dd
to
026dc72
Compare
|
Squashed and rebased EDIT: still some issues, hold on... |
|
There, please have a look |
| Message: message, | ||
| Redirect: redirect, | ||
| } | ||
| p.templates.ExecuteTemplate(rw, "error_with_redirect.html", t) |
There was a problem hiding this comment.
Please handle the error this could produce as it is done in ErrorPage
| logger.Fatalf("failed parsing template %s", err) | ||
| } | ||
|
|
||
| t, err = t.Parse(`{{define "error.html"}}` + header + ` |
There was a problem hiding this comment.
I wonder if we should extend the template to take the button link and the button message and use a single error template, WDYT?
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
@jonerer bump? :) |
|
@mlazowik hello! I'm sorry I didn't finish this up; I went on parental leave (still am), so don't have time or access to the cluster to finish it. FWIW we are (or at least were, idk what the current state is) using this in prod. It did the job as intended, but introduced a few rare HTTP 500. Those were probably related to some other error state's HTML generation being broken by this change. So the current state is "almost good enough to merge, but not quite. and I personally will sadly not have time to take care of it in the foreseeable future". |
|
We will likely be unable to resurrect this from the grave. The HTML page rendering has been completely refactored. This PR will attempt to cleanup a little bit of the wonkiness around CSRF cookies: #967 |
|
Thanks for the update! I might look into this if it becomes too much of an annoyance for our users. |
Upon CSRF error, provide the user with a button to go back to the app, following the original redirect. Also add the option to automatically follow redirects in such conditions.
See issues #607 and #611
Description
In the case where we have a CSRF failure, we have either had a race condition, or the user has bookmarked the login page. Allowing the user to go back to the original redirection URL would bring them back to the target app. In the case where the authentication has completed (on another tab or window), this will resolve the issue. In the case where the user has not completed an authentcation (such as when they have bookmarked the OIDC login page), following a redirect back to the app will start a new authentication process, which hopefully succeeds this time.
Motivation and Context
#607 and #611
How Has This Been Tested?
I have tested by running oauth2-proxy locally on my machine, with AWS Cognito as a backend. I have not written automated tests yet, since I'm not sure if you agree that this is the correct way to solve the issue.
Checklist: