-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add $JUPYTERHUB_XSRF_ANONYMOUS_{IP_CIDRS|HEADERS} config for managing anonymous xsrf ids #4991
base: main
Are you sure you want to change the base?
Conversation
almost certainly proxy ips, which don't solve the problem anyway, and can change due to replicas
Does this potentially open up a vulnerability with CGNAT? |
…ID_HEADERS for influencing how anonymous xsrf tokens are computed including fully opting out of distinguishable xsrf ids for anonymous (login) requests
I believe |
I added a couple of environment variables $JUPYTERHUB_XSRF_ANONYMOUS_USE_IP and $JUPYTERHUB_XSRF_ANONYMOUS_ID_HEADERS to influence how the anonymous id is computed. I did this because it's influencing a top-level utility function and traitlets-style config will be a bit of a pain to get a Configurable instance properly loaded. But not impossible, if folks think standard config would be better here. The unique anonymous id can be fully disabled with: JUPYTERHUB_XSRF_ANONYMOUS_USE_IP=0
JUPYTERHUB_XSRF_ANONYMOUS_ID_HEADERS= in which case an xsrf cookie for one client is valid for all clients (assuming client is not authenticated), which opens a path to cookie tossing. |
jupyterhub/_xsrf_utils.py
Outdated
@@ -230,6 +234,17 @@ def check_xsrf_cookie(handler): | |||
) | |||
|
|||
|
|||
# allow disabling using ip in anonymous id | |||
_anonymous_use_ip = _bool_env("JUPYTERHUB_XSRF_ANONYMOUS_USE_IP", default=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use JUPYTERHUB_XSRF_ANONYMOUS_USE_IP
with False by default to avoid any security problem and keep the old behaviour?
I think this problem is more likely to occur in k8s deployment, so we can set this property to True on the chart side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. True means keeping the current behavior by default, where the ip is used, which is correct most of the time (including in most z2jh deployments). Only where proxy headers don't properly propagate does this problem come up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this change the default behaviour on internal networks, since all private IPs are treated as one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. I'm not particularly concerned about that case, since it only affects the non-portability of the xsrf cookies for login, i.e. for cookie tossing scenarios. I don't think that would really be an issue for private network deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't feel comfortable with treating all private IPs as one, but I'm not going to block merging this if others are happy.
As one possible alternative could we replace JUPYTERHUB_XSRF_ANONYMOUS_USE_IP
with something like JUPYTERHUB_XSRF_ANONYMOUS_<SOMETHING>_CIDRS
which defaults to empty (so no change to current mainline behaviour), but setting it to a CIDR would treat any IP in that CIDR as identical for the XSRF check? Then the equivalent of JUPYTERHUB_XSRF_ANONYMOUS_USE_IP=False
would be JUPYTERHUB_XSRF_ANONYMOUS_<SOMETHING>_CIDRS="0.0.0.0/0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented your suggestion as JUPYTERHUB_XSRF_ANONYMOUS_IP_CIDRS
. When a CIDR matches, the matching CIDR is used in the hash instead of the ip (so e.g. access from multiple flattened networks is still non-transferrable across networks), and it is now opt-in.
That means jupyterhub/zero-to-jupyterhub-k8s#3422 is no longer fixed by default, but it is fixable and we may be able to make it fixed by default in z2jh, I think.
almost certainly proxy ips, which don't solve the problem anyway, and can change due to multiple proxy replicas
maybe closes jupyterhub/zero-to-jupyterhub-k8s#3422 for most users. A case where it wouldn't is a proxy outside the cluster/LAN, where the proxy's own ip is a global ip. For that, we'd need a more full opt-out of using ips.