Description
Currently (v5.1.0), when a session is encrypted and the size of the session exceeds 4kb, there is logic within the proxy to split the cookie. This is required so that we do not exceed the maximum size of a cookie. As far as I'm aware this is guaranteed to happen when using Azure because of the size of the ID Tokens they generate, and it has caused a number of issues in the past.
There is an alternate solution however. By using redis as a session store (potentially we need to implement other alternate session stores? sqlite could be a good option for small deployments, dynamo or other cloud dbs for scaled deployments? Kubernetes CRDs for those running on Kubernetes), the larger sessions can be stored server side and the client need only keep track of a session ticket encrypted within the cookie, which is typically much smaller.
There are other additional benefits of server side session storage as well, eg you are less likely to have authentication fail during refreshes as the session can not go stale in a cookie.
My proposal would be that we remove this behaviour from the proxy in a couple of steps:
- First, introduce a warning that the cookie is being split
- Secondly either remove the splitting altogether or feature gate it behind a flag
- If feature gated, make a note that this is unsupported behaviour and that support will not be provided when using the feature
I would like to gather feedback from the community on this idea before we make a breaking change, so please post comments below
Related issues:
- Using some extra args with Azure results in error 400 and 500 #458
- Redirect issues with Envoy #350
- A Server Cookies Store for long cookies #138
- ClearSessionCookie doesn't work for splitted cookies #69
- OIDC Cookie refresh not working with nginx auth_request when cookies are split. #29
- Do not propagate the _oauth2_proxy cookie to the upstream #388
- Grafana auth proxy scenario not working for dashboard calls #266
Activity
steakunderscore commentedon Apr 10, 2020
I'm all for this change. I agree this is something that should for a few months/releases be a deprecation warning.
johejo commentedon Apr 14, 2020
Agree.
Large cookies are not a good choice as they may be limited by the L7 load balancer (PaaS).
Recently I faced the HTTP/2 enabled Azure Application Gateway blocks large header (it means large cookie).
https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-subscription-service-limits#application-gateway-limits
It may be possible to support full local boot modes other than Redis (eg in-memory session storage).
They don't share storage when scaled horizontally and should be warned because they can't be used in production environments, but I think it's a good option for standalone use cases.
Also, sqlite3 driver for Go requires CGO, so some people might not like it.
https://github.com/mattn/go-sqlite3
JoelSpeed commentedon Apr 18, 2020
Thanks for your thoughts @johejo :)
I think we should come up with a deprecation plan for this, we currently have a warning in the proxy from the next release that warns that the cookies are being split, should we mark this as deprecated in the same release or wait for a patch release to do that?
I wonder if there are other ways to achieve this that wouldn't need CGO.
Alternative suggestions for potential storage implementations for sessions:
NickMeves commentedon Jun 12, 2020
Something on my mind after I looked at tuning up the redis session store that I might take a stab at reorganizing:
We could potentially split out all common logic dealing with encrypting the data, generated a session ticket for the cookie, and the like -- and make that common for all non-cookie session stores.
Then any new session store interfaces just need to know how to initialize and implement the Save/Load/Clear functionalities with a generic key/value (with the common logic dealing with making a key, encrypting, reading the cookie, etc).
Should make it easier to add in more session store types easily.
JoelSpeed commentedon Jun 14, 2020
@NickMeves I had similar thoughts before, and agree, would definitely make implementing different session stores easier. Seems like a reasonable refactoring project to me
47 remaining items