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

Session state's email stored in clear in the cookie #60

Closed
costelmoraru opened this issue Feb 13, 2019 · 4 comments
Closed

Session state's email stored in clear in the cookie #60

costelmoraru opened this issue Feb 13, 2019 · 4 comments

Comments

@costelmoraru
Copy link
Contributor

Expected Behavior

Some enterprises requests that no information should be exposed through the cookie value if cookie-secret is provided.

Current Behavior

However, the current implementation is just base64 encoding the email value, thus exposing the information. In the same cookie, the Session State encrypts the accessToken, IDToken, ExpiresOn and RefreshToken.

Possible Solution

During the EncryptString part of the session_state, when a cookie.Cipher is provided, encrypt also the account info made out of email and user together with the rest of the SessionState fields.
The same time, the Decode of the Session State should take into consideration and decrypt the email and user.

Steps to Reproduce (for bugs)

  1. Configure the oauth2_proxy with a cookie-secret and go through the auth process,
  2. Take the generated cookie cookie-name and apply a base64 decode and you will have the email value in clear.

Context

Some enterprises are now allowing exposure of any useful information in the cookies.

Your Environment

  • Version used: bitly's oauth2_proxy with @JoelSpeed's PR's
@ploxiln
Copy link
Contributor

ploxiln commented Feb 13, 2019

With the -cookie-httponly and -cookie-secure options, both of which are enabled by default, the browser will only send the cookie over https to the server running oauth2_proxy. For what it's worth.

@costelmoraru
Copy link
Contributor Author

In the session_state in the EncryptedString and DecodeSessionState functions, we can include the accountInfo() as encrypted instead of heaving them in clear.

If you agree I can do a PR with this fix.

@tlawrie, @JoelSpeed

@JoelSpeed
Copy link
Member

With the -cookie-httponly and -cookie-secure options, both of which are enabled by default, the browser will only send the cookie over https to the server running oauth2_proxy. For what it's worth.

@costelmoraru What is the problem with this approach? In this case the cookies are only available by the browser to be sent over HTTPS (so no man in the middle reading the cookie) and can't be read by client-side scripts (so no malicious scripts finding anything out), the only real way to see the content would be on the user's machine using something like developer tools? But I would expect the user would know their email anyway?

I appreciate enterprises can be quite strict with their security requirements but I am struggling to see the security flaw here

Your proposed solution does however seem sensible

@costelmoraru
Copy link
Contributor Author

Issue closed by the PR #120 .

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

3 participants