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

[1st & important] Fix domains and encryption key setup of securecookies with claims #29

Merged
merged 1 commit into from
May 8, 2020

Conversation

k3a
Copy link

@k3a k3a commented May 6, 2020

Mainly fixes #23, fixes #25 by configuring Domains and other options of session store.

Additionally, enabling RBAC previously required SESSION_KEY to be set to a non-empty value. Enabling RBAC and not setting SESSION_KEY were silently ignored, resulting with no group claims cookie produced - effectively a bug.

Next, SESSION_KEY was documented as A session key used to encrypt browser sessions which was not true. Gorilla's NewCookieStore says that the argument is a key or pair of keys. If only one key is passed, that is considered as only having the first key from the pair set, with the second key being empty. Keys are set in pairs because the first one of them is authentication key and the optional second one is encryption key. Multiple pairs can be set to allow key rotation. So the original implementation just setting the SESSION_KEY was actually setting it as the authentication key, preventing cookie value to be altered by a malicious actor, not encrypting the cookie as the config documentation string said - another bug.

This commit changes the behavior so that SECRET is used for authentication and SESSION_KEY is used for encryption as originally documented.

Idea to discuss: It would be possible to completely remove SESSION_KEY config field and use a single-key SECRET argument to NewCookieStore without encrypting the claims cookie. In the reality there is nothing secret in the groups claim so encryption is not really necessary. But having it encrypted (and using the pull request as-is) could allow more data (private eventually) to be stored in the session store.

all tests are ok as before (I intentionally left the %w bug there as #27 fixes this)

@k3a k3a changed the title Fix domains and encryption key setup of securecookies with claims [1st & important] Fix domains and encryption key setup of securecookies with claims May 7, 2020
@jr0d
Copy link
Contributor

jr0d commented May 8, 2020

@k3a thanks for picking up my slack. Also, great catch on the session key issue. I was not too concerned about encrypting the session after a behavior change where we no longer needed to store the users id-token; and opted to only store the group claims.

Copy link
Contributor

@jr0d jr0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@jr0d jr0d merged commit 06a0eb7 into mesosphere:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants