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

Cookie persistence, browser session cookie suggestion #1287

Closed
mhlyak opened this issue Jul 29, 2021 · 9 comments
Closed

Cookie persistence, browser session cookie suggestion #1287

mhlyak opened this issue Jul 29, 2021 · 9 comments

Comments

@mhlyak
Copy link

mhlyak commented Jul 29, 2021

When closing the browser after a successful user sign in, the session is still alive for the time the oauth2-proxy cookie has not expired and tokens in session are still valid.

Expected Behavior

There could be an additional "browser session enabled" configuration. When the browser is closed (or when the browser defines the "current session" ends), the oauth2-proxy user session should also expire.

Current Behavior

Currently, the user is still logged in if the browser is closed and reopened (if you navigate to /oauth2/userinfo page after reopening the browser, you get signed-in user data).
But, if oauth2-proxy is integrated with a provider that supports browser session and a user navigates to the /oauth2/sign_in page, the user needs to sign in again.
With a possibility to enable browser session on oauth2-proxy, a user should not be signed in after reopening the browser (or when the browser defines the "current session" ends).

Possible Solution

There could be an additional "browser session enabled" configuration for oauth2-proxy.

Steps to Reproduce (for bugs)

  1. In the browser, navigate to the oauth2-proxy sign_in page (/oauth2/sign_in) and sign in
  2. Close the browser
  3. In the same browser, navigate to the oauth2-proxy userinfo page (/oauth2/userinfo) and you can see that you are already logged in
  4. Navigate to the oauth2-proy sign_in page (/oauth2/sign_in) and if the provider supports the browser session (e.g. Keycloak), you need to sign in again

Context

For example, sign into the first app behind the oauth2-proxy with user 1.
Reopen the browser and navigate to another app protected with the same provider that supports the browser session and sign in with user 2.
When navigating back to the first app, you are still signed in with user 1. Navigate to oauth2-proxy sign in page and you will be signed in as user 2.
This change would be required for security purposes. User 1 would have thought to be logged out from the first app where he, in fact, wouldn't have been.
As such, user 2 could steal the identity of user 1 in the first app.

Your Environment

We are using oidc provider connected with Keycloak of version 8.0.2.
Oauth2-proxy cookie expire is set to 20m and cookie refresh to 4m.

  • Version used: 7.1.3
@JoelSpeed
Copy link
Member

I think this is a problem in the way we set up the cookie expiration, but it could be adapted IMO to set the correct options so that the cookie expires on browser session closing.

Does anyone have time to look into adding this functionality?

Are there any concerns we might have about the security aspect of this? Eg if a user never closes their browser, is that an issue?

@ssitsp
Copy link

ssitsp commented Jul 30, 2021

It's problem because in such way as you set up your session cookie, expiration session has lifetime as configured in your product. This is done with persistent session cookie that survive browser restart and is not constrained with browser session.
If you change the session maintenance cookie from persistent to browser session cookie you louse the ability to configure session lifetime.
This problem has to be solved with 2 cookies.
One persistent (as is currently your session cookie) that defines configured session life time and another cookie that is actually browser session cookie that reacts on browser session shutdown. When browser session cookie is not accessible your app should clean the session. When it'is accessible and there is no more persistent session cookie, your app should clean the session.
You can even provide configuration to enable/disable such behaviour.
The problem can be reproduced in cookie and redis session setup of your product.

@spetar
Copy link

spetar commented Aug 19, 2021

Hello @JoelSpeed is there any plans to implement this feature? There is a way to implement this in a way that it has no security aspect mentioned above. Who can add this functionality ? ("Does anyone have time to look into adding this functionality?") Anyone ?

@JoelSpeed
Copy link
Member

Just wondering if we actually do need a second cookie for this, or whether we can just adjust the existing cookie we have?

The code that does our session refreshing doesn't actually rely on the cookie expiration at all, so I don't think this would be affected. I think the issue is the convolution of the options. We should have explicit cookie options and session options for refreshing, but for now we don't.

I think we could achieve the desired effect simply by adding a new option that allows the session cookie to be set without an expiry, meaning it should clear when the browser is cleared right?

@kfox1111
Copy link

Are there any drawbacks to just dropping a refresh token without telling the oidc server you don't need it anymore?

@JoelSpeed
Copy link
Member

Are there any drawbacks to just dropping a refresh token without telling the oidc server you don't need it anymore?

None that I'm aware of, how does that fit in here? What ya thinking? 🤔

@kfox1111
Copy link

well, having the second cookie would allow oauth2 to detect that the session has expired and would be able to tell the upstream oidc provider it no longer needed the refresh token. Only thing I can think of where 2 cookies might be useful.

@JoelSpeed
Copy link
Member

Interesting idea, I wonder if that's something that's even possible (I've not seen that before at least) 🤔

From my experience with these kinds of things at least, they keep the refresh token around until you next sign in, at which point you get a new token, so while this could be a nice optimisation, I don't think it's something we need to do. I guess we have to weigh up the complexity with the benefit

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

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

No branches or pull requests

5 participants