search

mesosphere / traefik-forward-auth

218 stars 47 forks source link

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

Closed k3a closed 4 years ago

k3a commented 4 years ago

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)

jr0d commented 4 years ago

@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.