From b0ae97fd323fb9124e81299ccf2f4ab2d0731829 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Sat, 10 Feb 2024 15:55:29 +0100 Subject: [PATCH 01/43] Add Authenticator.allow_existing_users Configuring `Authenticator.allowed_users` truthy makes other existing users in JupyterHub's database be allowed access, this could come as a surprise. This new config is meant to help avoid such surprise. With this new config, a JupyterHub admin is able to directly declare if the existing users in JupyterHub's database is to be granted access or not. If `allow_existing_users` isn't explicity set, the default value will be computed to True or False depending on if `allowed_users` is Truthy, which makes the introduction of this config a non-breaking change. This configuration was initially introduced in jupyterhub/oauthenticator via https://github.com/jupyterhub/oauthenticator/pull/631, and is with this PR being upstreamed to the base Authenticator class. --- jupyterhub/auth.py | 57 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 6a1f01a8..bf884ee9 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -144,6 +144,54 @@ class Authenticator(LoggingConfigurable): """ ).tag(config=True) + allow_existing_users = Bool( + False, # the default value is computed by a function below + config=True, + help=""" + Allow existing users to login. + + Defaults to True if `allowed_users` is set for historical reasons, and + False otherwise. + + An existing user is a user in JupyterHub's database of users, and it + includes all users that has previously logged in. + + .. warning:: + + Before enabling this you should review the existing users in the + JupyterHub admin panel at `/hub/admin`. You may find users existing + there because they have once been declared in config such as + `allowed_users`, or once been allowed to sign in. + + .. warning:: + + When this is enabled and you are to remove access for one or more + users allowed via other config options, you must make sure that they + are not part of the database of users still. This can be tricky to do + if you stop allowing a group of externally managed users for example. + + With this enabled, JupyterHub admin users can visit `/hub/admin` or use + JupyterHub's REST API to add and remove users as a way to allow or + revoke access. + + The username for existing users must match the normalized username + returned by the authenticator. + + .. versionadded:: 5.0 + """, + ) + + @default("allow_existing_users") + def _allow_existing_users_default(self): + """ + Computes the default value of allow_existing_users based on if + allowed_users to align with original behavior not introduce a breaking + change. + """ + if self.allowed_users: + return True + return False + blocked_users = Set( help=""" Set of usernames that are not allowed to log in. @@ -682,7 +730,8 @@ class Authenticator(LoggingConfigurable): This method may be a coroutine. - By default, this just adds the user to the allowed_users set. + By default, this adds the user to the allowed_users set if + allow_existing_users is true. Subclasses may do more extensive things, such as adding actual unix users, but they should call super to ensure the allowed_users set is updated. @@ -690,12 +739,16 @@ class Authenticator(LoggingConfigurable): Note that this should be idempotent, since it is called whenever the hub restarts for all users. + .. versionchanged:: 5.0 + Now adds users to the allowed_users set if allow_existing_users is + True, instead of if allowed_users was truthy. + Args: user (User): The User wrapper object """ if not self.validate_username(user.name): raise ValueError("Invalid username: %s" % user.name) - if self.allowed_users: + if self.allow_existing_users: self.allowed_users.add(user.name) def delete_user(self, user): From 0d427338a16ae3f4251fe46711686f0441b6cce9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 13 Mar 2024 15:00:08 +0100 Subject: [PATCH 02/43] add Authenticator.allow_all and test coverage for allow_all and allow_existing_users interactions PAMAuthenticator.allowed_groups is no longer mutually exclusive with allowed_users --- jupyterhub/auth.py | 112 +++++++++++++++------- jupyterhub/tests/test_auth.py | 172 +++++++++++++++++++++++++++++++++- 2 files changed, 251 insertions(+), 33 deletions(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index bf884ee9..70092fd9 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -132,7 +132,7 @@ class Authenticator(LoggingConfigurable): Use this to limit which authenticated users may login. Default behavior: only users in this set are allowed. - + If empty, does not perform any restriction, in which case any authenticated user is allowed. @@ -144,8 +144,54 @@ class Authenticator(LoggingConfigurable): """ ).tag(config=True) + allow_all = Bool( + # dynamic default computed from allowed_users + config=True, + help=""" + Any users who can successfully authenticate are allowed to access JupyterHub. + + For backward-compatibility, this is True by default if `allowed_users` is unspecified, + False if `allowed_users` is specified. + + Authenticator subclasses may override the default with e.g.:: + + @default("allow_all") + def _default_allow_all(self): + return False + + # or + + @default("allow_all") + def _default_allow_all(self): + if self.allowed_users or self.allowed_groups: + return False + else: + return True + + False is a good idea when the group of users who can authenticate is typically larger + than the group users who should have access (e.g. OAuth providers). + + Authenticator subclasses that define additional sources of `allow` config + beyond `allowed_users` should specify a default value for allow_all, + either always False or `if not any allow_config`. + + This is checked inside `check_allowed`, so subclasses that override `check_allowed` + must explicitly check `allow_all` for it to have any effect. + This is for safety, to ensure that no Authenticator subclass gets unexpected behavior from `allow_all`. + + .. versionadded:: 5.0 + """, + ) + + @default("allow_all") + def _default_allow_all(self): + if self.allowed_users: + return False + else: + return True + allow_existing_users = Bool( - False, # the default value is computed by a function below + # dynamic default computed from allowed_users config=True, help=""" Allow existing users to login. @@ -153,29 +199,27 @@ class Authenticator(LoggingConfigurable): Defaults to True if `allowed_users` is set for historical reasons, and False otherwise. - An existing user is a user in JupyterHub's database of users, and it - includes all users that has previously logged in. + With this enabled, all users present in the JupyterHub database are allowed to login. + This has the effect of any user who has _previously_ been allowed to login + via any means will continue to be allowed until the user is deleted via the /hub/admin page + or REST API. .. warning:: Before enabling this you should review the existing users in the JupyterHub admin panel at `/hub/admin`. You may find users existing - there because they have once been declared in config such as - `allowed_users`, or once been allowed to sign in. + there because they have previously been declared in config such as + `allowed_users` or allowed to sign in. .. warning:: - When this is enabled and you are to remove access for one or more - users allowed via other config options, you must make sure that they - are not part of the database of users still. This can be tricky to do + When this is enabled and you wish to remove access for one or more + users previously allowed, you must make sure that they + are removed from the jupyterhub database. This can be tricky to do if you stop allowing a group of externally managed users for example. With this enabled, JupyterHub admin users can visit `/hub/admin` or use - JupyterHub's REST API to add and remove users as a way to allow or - revoke access. - - The username for existing users must match the normalized username - returned by the authenticator. + JupyterHub's REST API to add and remove users to manage who can login. .. versionadded:: 5.0 """, @@ -520,8 +564,7 @@ class Authenticator(LoggingConfigurable): web.HTTPError(403): Raising HTTPErrors directly allows customizing the message shown to the user. """ - if not self.allowed_users: - # No allowed set means any name is allowed + if self.allow_all: return True return username in self.allowed_users @@ -725,8 +768,8 @@ class Authenticator(LoggingConfigurable): """Hook called when a user is added to JupyterHub This is called: - - When a user first authenticates - - When the hub restarts, for all users. + - When a user first authenticates, _after_ all allow and block checks have passed + - When the hub restarts, for all users in the database (i.e. users previously allowed) This method may be a coroutine. @@ -740,14 +783,17 @@ class Authenticator(LoggingConfigurable): for all users. .. versionchanged:: 5.0 - Now adds users to the allowed_users set if allow_existing_users is - True, instead of if allowed_users was truthy. + Now adds users to the allowed_users set if allow_all is False, + instead of if allowed_users is not empty. Args: user (User): The User wrapper object """ if not self.validate_username(user.name): raise ValueError("Invalid username: %s" % user.name) + # this is unnecessary if allow_all is True, + # but skipping this when allow_all is False breaks backward-compatibility + # for Authenticator subclasses that may not yet understand allow_all if self.allow_existing_users: self.allowed_users.add(user.name) @@ -955,23 +1001,25 @@ class LocalAuthenticator(Authenticator): help=""" Allow login from all users in these UNIX groups. - If set, allowed username set is ignored. + .. versionchanged:: 5.0 + `allowed_groups` may be specified together with allowed_users, + to grant access by group OR name. """ ).tag(config=True) - @observe('allowed_groups') - def _allowed_groups_changed(self, change): - """Log a warning if mutually exclusive user and group allowed sets are specified.""" - if self.allowed_users: - self.log.warning( - "Ignoring Authenticator.allowed_users set because Authenticator.allowed_groups supplied!" - ) + @default("allow_all") + def _allow_all_default(self): + if self.allowed_users or self.allowed_groups: + # if any allow config is specified, default to False + return False + return True def check_allowed(self, username, authentication=None): - if self.allowed_groups: - return self.check_allowed_groups(username, authentication) - else: - return super().check_allowed(username, authentication) + if self.allow_all: + return True + if self.check_allowed_groups(username, authentication): + return True + return super().check_allowed(username, authentication) def check_allowed_groups(self, username, authentication=None): """ diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index f9a228ea..a298283b 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -3,12 +3,13 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import logging +from itertools import chain from unittest import mock from urllib.parse import urlparse import pytest from requests import HTTPError -from traitlets import Any +from traitlets import Any, Tuple from traitlets.config import Config from jupyterhub import auth, crypto, orm @@ -593,3 +594,172 @@ async def test_auth_managed_groups( assert not app.db.dirty groups = sorted(g.name for g in user.groups) assert groups == expected_refresh_groups + + +@pytest.mark.parametrize( + "allowed_users, allow_all, allow_existing_users", + [ + ('specified', False, True), + ('', True, False), + ], +) +def test_allow_all_defaults(app, user, allowed_users, allow_all, allow_existing_users): + if allowed_users: + allowed_users = set(allowed_users.split(',')) + else: + allowed_users = set() + authenticator = auth.Authenticator(allowed_users=allowed_users) + assert authenticator.allow_all == allow_all + assert authenticator.allow_existing_users == allow_existing_users + + # user was already in the database + # this happens during hub startup + authenticator.add_user(user) + if allowed_users: + assert user.name in authenticator.allowed_users + else: + authenticator.allowed_users == set() + + assert authenticator.check_allowed("specified") + assert authenticator.check_allowed(user.name) + + +@pytest.mark.parametrize("allow_all", [None, True, False]) +@pytest.mark.parametrize("allow_existing_users", [None, True, False]) +@pytest.mark.parametrize("allowed_users", ["existing", ""]) +def test_allow_existing_users( + app, user, allowed_users, allow_all, allow_existing_users +): + if allowed_users: + allowed_users = set(allowed_users.split(',')) + else: + allowed_users = set() + authenticator = auth.Authenticator( + allowed_users=allowed_users, + ) + if allow_all is None: + # default allow_all + allow_all = authenticator.allow_all + else: + authenticator.allow_all = allow_all + if allow_existing_users is None: + # default allow_all + allow_existing_users = authenticator.allow_existing_users + else: + authenticator.allow_existing_users = allow_existing_users + + # first, nobody in the database + assert authenticator.check_allowed("newuser") == allow_all + + # user was already in the database + # this happens during hub startup + authenticator.add_user(user) + if allow_existing_users or allow_all: + assert authenticator.check_allowed(user.name) + else: + assert not authenticator.check_allowed(user.name) + for username in allowed_users: + assert authenticator.check_allowed(username) + + assert authenticator.check_allowed("newuser") == allow_all + + +@pytest.mark.parametrize("allow_all", [True, False]) +@pytest.mark.parametrize("allow_existing_users", [True, False]) +def test_allow_existing_users_first_time(user, allow_all, allow_existing_users): + # make sure that calling add_user doesn't change results + authenticator = auth.Authenticator( + allow_all=allow_all, + allow_existing_users=allow_existing_users, + ) + allowed_before_one = authenticator.check_allowed(user.name) + allowed_before_two = authenticator.check_allowed("newuser") + # add_user is called after successful login + # it shouldn't change results (e.g. by switching .allowed_users from empty to non-empty) + if allowed_before_one: + authenticator.add_user(user) + assert authenticator.check_allowed(user.name) == allowed_before_one + assert authenticator.check_allowed("newuser") == allowed_before_two + + +class AllowAllIgnoringAuthenticator(auth.Authenticator): + """Test authenticator with custom check_allowed + + not updated for allow_all, allow_existing_users + + Make sure new config doesn't break backward-compatibility + or grant unintended access for Authenticators written before JupyterHub 5. + """ + + allowed_letters = Tuple(config=True, help="Initial letters to allow") + + def check_allowed(self, username, auth=None): + if not self.allowed_users and not self.allowed_letters: + # this subclass doesn't know about the JupyterHub 5 allow_all config + # no allow config, allow all! + return True + if self.allowed_users and username in self.allowed_users: + return True + if self.allowed_letters and username.startswith(self.allowed_letters): + return True + return False + + +# allow_all is not recognized by Authenticator subclass +# make sure it doesn't make anything more permissive, at least +@pytest.mark.parametrize("allow_all", [True, False, None]) +@pytest.mark.parametrize( + "allowed_users, allowed_letters, allow_existing_users, allowed, not_allowed", + [ + ("", "", None, "anyone,should-be,allowed,existing", ""), + ("", "a,b", None, "alice,bebe", "existing,other"), + ("", "a,b", False, "alice,bebe", "existing,other"), + ("", "a,b", True, "alice,bebe,existing", "other"), + ("specified", "a,b", None, "specified,alice,bebe,existing", "other"), + ("specified", "a,b", False, "specified,alice,bebe", "existing,other"), + ("specified", "a,b", True, "specified,alice,bebe,existing", "other"), + ], +) +def test_authenticator_without_allow_all( + app, + allowed_users, + allowed_letters, + allow_existing_users, + allowed, + not_allowed, + allow_all, +): + kwargs = {} + if allow_all is not None: + kwargs["allow_all"] = allow_all + if allow_existing_users is not None: + kwargs["allow_existing_users"] = allow_existing_users + if allowed_users: + kwargs["allowed_users"] = set(allowed_users.split(',')) + if allowed_letters: + kwargs["allowed_letters"] = tuple(allowed_letters.split(',')) + + authenticator = AllowAllIgnoringAuthenticator(**kwargs) + + # load one user from db + existing_user = add_user(app.db, app, name="existing") + authenticator.add_user(existing_user) + + if allowed: + allowed = allowed.split(",") + if not_allowed: + not_allowed = not_allowed.split(",") + + expected_allowed = sorted(allowed) + expected_not_allowed = sorted(not_allowed) + to_check = list(chain(expected_allowed, expected_not_allowed)) + are_allowed = [] + are_not_allowed = [] + for username in to_check: + if authenticator.check_allowed(username): + are_allowed.append(username) + else: + are_not_allowed.append(username) + + assert are_allowed == expected_allowed + assert are_not_allowed == expected_not_allowed From e1e34a14a2ce0f10582d7a152b1351f61596df67 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 18 Mar 2024 16:03:32 +0100 Subject: [PATCH 03/43] update docs for allow_all, allow_existing_users --- docs/source/reference/authenticators.md | 119 ++++++++++++++++-- .../authenticators-users-basics.md | 63 +++++++--- 2 files changed, 154 insertions(+), 28 deletions(-) diff --git a/docs/source/reference/authenticators.md b/docs/source/reference/authenticators.md index 045e9605..1508a092 100644 --- a/docs/source/reference/authenticators.md +++ b/docs/source/reference/authenticators.md @@ -37,14 +37,14 @@ A [generic implementation](https://github.com/jupyterhub/oauthenticator/blob/mas ## The Dummy Authenticator When testing, it may be helpful to use the -{class}`jupyterhub.auth.DummyAuthenticator`. This allows for any username and -password unless if a global password has been set. Once set, any username will +{class}`~.jupyterhub.auth.DummyAuthenticator`. This allows for any username and +password unless a global password has been set. Once set, any username will still be accepted but the correct password will need to be provided. ## Additional Authenticators -A partial list of other authenticators is available on the -[JupyterHub wiki](https://github.com/jupyterhub/jupyterhub/wiki/Authenticators). +Additional authenticators can be found on GitHub +by searching for [topic:jupyterhub topic:authenticator](https://github.com/search?q=topic%3Ajupyterhub%20topic%3Aauthenticator&type=repositories). ## Technical Overview of Authentication @@ -54,9 +54,9 @@ The base authenticator uses simple username and password authentication. The base Authenticator has one central method: -#### Authenticator.authenticate method +#### Authenticator.authenticate - Authenticator.authenticate(handler, data) +{meth}`.Authenticator.authenticate` This method is passed the Tornado `RequestHandler` and the `POST data` from JupyterHub's login form. Unless the login form has been customized, @@ -81,7 +81,7 @@ Writing an Authenticator that looks up passwords in a dictionary requires only overriding this one method: ```python -from IPython.utils.traitlets import Dict +from traitlets import Dict from jupyterhub.auth import Authenticator class DictionaryAuthenticator(Authenticator): @@ -136,7 +136,7 @@ To only allow usernames that start with 'w': c.Authenticator.username_pattern = r'w.*' ``` -### How to write a custom authenticator +## How to write a custom authenticator You can use custom Authenticator subclasses to enable authentication via other mechanisms. One such example is using [GitHub OAuth][]. @@ -148,11 +148,6 @@ and {meth}`.Authenticator.post_spawn_stop`, are hooks that can be used to do auth-related startup (e.g. opening PAM sessions) and cleanup (e.g. closing PAM sessions). -See a list of custom Authenticators [on the wiki](https://github.com/jupyterhub/jupyterhub/wiki/Authenticators). - -If you are interested in writing a custom authenticator, you can read -[this tutorial](http://jupyterhub-tutorial.readthedocs.io/en/latest/authenticators.html). - ### Registering custom Authenticators via entry points As of JupyterHub 1.0, custom authenticators can register themselves via @@ -188,6 +183,104 @@ Additionally, configurable attributes for your authenticator will appear in jupyterhub help output and auto-generated configuration files via `jupyterhub --generate-config`. +(authenticator-allow)= + +### Allowing access + +When dealing with logging in, there are generally two _separate_ steps: + +authentication +: identifying who is logged in, and + +authorization +: deciding whether an authenticated user is logged in + +{meth}`Authenticator.authenticate` is responsible for authenticating users. +It is perfectly fine in the simplest cases for `Authenticator.authenticate` to be responsible for authentication _and_ authorization, +in which case `authenticate` may return `None` if the user is not authorized. + +However, Authenticators also have have two methods {meth}`~.Authenticator.check_allowed` and {meth}`~.Authenticator.check_blocked_users`, which are called after successful authentication to further check if the user is allowed. + +If `check_blocked_users()` returns False, authorization stops and the user is not allowed. + +If `check_allowed()` returns True, authorization proceeds. + +:::{versionadded} 5.0 +{attr}`Authenticator.allow_all` and {attr}`Authenticator.allow_existing_users` are new in JupyterHub 5.0. + +By default, `allow_all` is True when `allowed_users` is empty, +and `allow_existing_users` is True when `allowed_users` is not empty. +This is to ensure backward-compatibility, but subclasses are free to pick more restrictive defaults. +::: + +### Overriding `check_allowed` + +The base implementation of {meth}`~.Authenticator.check_allowed` checks: + +- if `allow_all` is True, return True +- if username is in the `allowed_users` set, return True +- else return False + +If a custom Authenticator defines additional sources of `allow` configuration, +such as membership in a group or other information, +it should override `check_allowed` to account for this. +`allow_` configuration should generally be _additive_, +i.e. if permission is granted by _any_ allow configuration, +a user should be authorized. + +:::{note} +For backward-compatibility, it is the responsibility of `Authenticator.check_allowed()` to check `.allow_all`. +This is to avoid the backward-compatible default values from granting permissions unexpectedly. +::: + +If an Authenticator defines additional `allow` configuration, it must at least: + +1. override `check_allowed`, and +2. override the default for `allow_all` + +The default for `allow_all` in a custom authenticator should be one of `False` or a dynamic default matching something like `if not any allow configuration specified`. +False is recommended for authenticators which source much larger pools of users than are _typically_ allowed to access a Hub (e.g. generic OAuth providers like Google, GitHub, etc.). + +For example, here is how `PAMAuthenticator` extends the base class to add `allowed_groups`: + +```python +from traitlets import default + +@default("allow_all") +def _allow_all_default(self): + if self.allowed_users or self.allowed_groups: + # if any allow config is specified, default to False + return False + return True + +def check_allowed(self, username, authentication=None): + if self.allow_all: + return True + if self.check_allowed_groups(username, authentication): + return True + return super().check_allowed(username, authentication) +``` + +Important points to note: + +- overriding the default for `allow_all` is required to avoid `allow_all` being True when `allowed_groups` is specified, but `allowed_users` is not. +- `allow_all` must be checked inside `check_allowed` +- `allowed_groups` strictly expands who is authorized, + it does not apply restrictions `allowed_users`. + This is recommended for all `allow_` configuration added by Authenticators. + +#### Custom error messages + +Any of these authentication and authorization methods may + +```python +from tornado import web + +raise web.HTTPError(403, "informative message") +``` + +if you want to show a more informative login failure message rather than the generic one. + (authenticator-auth-state)= ### Authentication state diff --git a/docs/source/tutorial/getting-started/authenticators-users-basics.md b/docs/source/tutorial/getting-started/authenticators-users-basics.md index f2f2d2ae..4b2b4e2f 100644 --- a/docs/source/tutorial/getting-started/authenticators-users-basics.md +++ b/docs/source/tutorial/getting-started/authenticators-users-basics.md @@ -13,15 +13,25 @@ You can restrict which users are allowed to login with a set, ```python c.Authenticator.allowed_users = {'mal', 'zoe', 'inara', 'kaylee'} +c.Authenticator.allow_all = False +c.Authenticator.allow_existing_users = False ``` -Users in the `allowed_users` set are added to the Hub database when the Hub is -started. +Users in the `allowed_users` set are added to the Hub database when the Hub is started. ```{warning} -If this configuration value is not set, then **all authenticated users will be allowed into your hub**. +If `allowed_users` is not specified, then by default **all authenticated users will be allowed into your hub**, +i.e. `allow_all` defaults to True if neither `allowed_users` nor `allow_all` are set. ``` +:::{versionadded} 5.0 +{attr}`Authenticator.allow_all` and {attr}`Authenticator.allow_existing_users` are new in JupyterHub 5.0. + +By default, `allow_all` is True when `allowed_users` is empty, +and `allow_existing_users` is True when `allowed_users` is not empty. +This is to ensure backward-compatibility. +::: + ## One Time Passwords ( request_otp ) By setting `request_otp` to true, the login screen will show and additional password input field @@ -42,7 +52,7 @@ c.Authenticator.otp_prompt = 'Google Authenticator:' ```{note} As of JupyterHub 2.0, the full permissions of `admin_users` should not be required. -Instead, you can assign [roles](define-role-target) to users or groups +Instead, it is best to assign [roles](define-role-target) to users or groups with only the scopes they require. ``` @@ -68,26 +78,49 @@ group. For example, we can let any user in the `wheel` group be an admin: c.PAMAuthenticator.admin_groups = {'wheel'} ``` -## Give admin access to other users' notebook servers (`admin_access`) +## Give some users access to other users' notebook servers -Since the default `JupyterHub.admin_access` setting is `False`, the admins -do not have permission to log in to the single user notebook servers -owned by _other users_. If `JupyterHub.admin_access` is set to `True`, -then admins have permission to log in _as other users_ on their -respective machines for debugging. **As a courtesy, you should make -sure your users know if admin_access is enabled.** +The `access:servers` scope can be granted to users to give them permission to visit other users' servers. +For example, to give members of the `teachers` group access to the servers of members of the `students` group: + +```python +c.JupyterHub.load_roles = [ + { + "name": "teachers", + "scopes": [ + "admin-ui", + "list:users", + "access:servers!group=students", + ], + "groups": ["teachers"], + } +] +``` + +By default, only the deprecated `admin` role has global `access` permissions. +**As a courtesy, you should make sure your users know if admin access is enabled.** ## Add or remove users from the Hub Users can be added to and removed from the Hub via the admin -panel or the REST API. When a user is **added**, the user will be -automatically added to the `allowed_users` set and database. Restarting the Hub -will not require manually updating the `allowed_users` set in your config file, +panel or the REST API. + +To enable this behavior, set: + +```python +c.Authenticator.allow_existing_users = True +``` + +When a user is **added**, the user will be +automatically added to the `allowed_users` set and database. +If `allow_existing_users` is True, restarting the Hub will not require manually updating the `allowed_users` set in your config file, as the users will be loaded from the database. +If `allow_existing_users` is False, users not granted access by configuration such as `allowed_users` will not be permitted to login, +even if they are present in the database. After starting the Hub once, it is not sufficient to **remove** a user from the allowed users set in your config file. You must also remove the user -from the Hub's database, either by deleting the user from JupyterHub's +from the Hub's database, either by deleting the user via JupyterHub's admin page, or you can clear the `jupyterhub.sqlite` database and start fresh. From 74455d6337573b92e83482af9d778576a1664d50 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Tue, 19 Mar 2024 12:50:03 +0000 Subject: [PATCH 04/43] Bump required Python version in contributing setup --- docs/source/contributing/setup.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/source/contributing/setup.md b/docs/source/contributing/setup.md index a00cc131..eaea5957 100644 --- a/docs/source/contributing/setup.md +++ b/docs/source/contributing/setup.md @@ -12,7 +12,7 @@ development. ### Install Python JupyterHub is written in the [Python](https://python.org) programming language and -requires you have at least version 3.6 installed locally. If you haven’t +requires you have at least version 3.8 installed locally. If you haven’t installed Python before, the recommended way to install it is to use [Miniforge](https://github.com/conda-forge/miniforge#download). @@ -59,7 +59,7 @@ a more detailed discussion. python -V ``` - This should return a version number greater than or equal to 3.6. + This should return a version number greater than or equal to 3.8. ```bash npm -v From 83ce6d3f6bcaeb9f38de44ebc5f441e75f395123 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 19 Mar 2024 12:55:10 +0100 Subject: [PATCH 05/43] forward-port 4.1.0 --- .github/workflows/test.yml | 3 + docs/source/explanation/websecurity.md | 177 ++++++++++++++-- docs/source/reference/changelog.md | 51 +++++ jupyterhub/_xsrf_utils.py | 210 +++++++++++++++++++ jupyterhub/apihandlers/base.py | 11 +- jupyterhub/app.py | 22 ++ jupyterhub/handlers/base.py | 102 +++++++-- jupyterhub/services/auth.py | 241 +++++++++++++++++++-- jupyterhub/singleuser/extension.py | 22 +- jupyterhub/singleuser/mixins.py | 24 +-- jupyterhub/spawner.py | 5 + jupyterhub/tests/browser/test_browser.py | 253 +++++++++++++++++++++-- jupyterhub/tests/mocking.py | 25 ++- jupyterhub/tests/test_api.py | 11 +- jupyterhub/tests/test_pages.py | 26 ++- jupyterhub/tests/test_services_auth.py | 6 +- jupyterhub/tests/test_singleuser.py | 37 +++- jupyterhub/tests/utils.py | 13 +- jupyterhub/user.py | 3 + jupyterhub/utils.py | 16 ++ 20 files changed, 1122 insertions(+), 136 deletions(-) create mode 100644 jupyterhub/_xsrf_utils.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dd551181..98b0fe4b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -103,6 +103,9 @@ jobs: subset: singleuser - python: "3.11" browser: browser + - python: "3.11" + subdomain: subdomain + browser: browser - python: "3.12" main_dependencies: main_dependencies diff --git a/docs/source/explanation/websecurity.md b/docs/source/explanation/websecurity.md index a78486f0..854ff1f1 100644 --- a/docs/source/explanation/websecurity.md +++ b/docs/source/explanation/websecurity.md @@ -16,7 +16,8 @@ works. JupyterHub is designed to be a _simple multi-user server for modestly sized groups_ of **semi-trusted** users. While the design reflects serving -semi-trusted users, JupyterHub can also be suitable for serving **untrusted** users. +semi-trusted users, JupyterHub can also be suitable for serving **untrusted** users, +but **is not suitable for untrusted users** in its default configuration. As a result, using JupyterHub with **untrusted** users means more work by the administrator, since much care is required to secure a Hub, with extra caution on @@ -56,30 +57,63 @@ ensure that: If any additional services are run on the same domain as the Hub, the services **must never** display user-authored HTML that is neither _sanitized_ nor _sandboxed_ -(e.g. IFramed) to any user that lacks authentication as the author of a file. +to any user that lacks authentication as the author of a file. + +### Sharing access to servers + +Because sharing access to servers (via `access:servers` scopes or the sharing feature in JupyterHub 5) by definition means users can serve each other files, enabling sharing is not suitable for untrusted users without also enabling per-user domains. + +JupyterHub does not enable any sharing by default. ## Mitigate security issues The several approaches to mitigating security issues with configuration options provided by JupyterHub include: -### Enable subdomains +### Enable user subdomains JupyterHub provides the ability to run single-user servers on their own -subdomains. This means the cross-origin protections between servers has the -desired effect, and user servers and the Hub are protected from each other. A -user's single-user server will be at `username.jupyter.mydomain.com`. This also -requires all user subdomains to point to the same address, which is most easily -accomplished with wildcard DNS. Since this spreads the service across multiple -domains, you will need wildcard SSL as well. Unfortunately, for many -institutional domains, wildcard DNS and SSL are not available. **If you do plan -to serve untrusted users, enabling subdomains is highly encouraged**, as it -resolves the cross-site issues. +domains. This means the cross-origin protections between servers has the +desired effect, and user servers and the Hub are protected from each other. + +**Subdomains are the only way to reliably isolate user servers from each other.** + +To enable subdomains, set: + +```python +c.JupyterHub.subdomain_host = "https://jupyter.example.org" +``` + +When subdomains are enabled, each user's single-user server will be at e.g. `https://username.jupyter.example.org`. +This also requires all user subdomains to point to the same address, +which is most easily accomplished with wildcard DNS, where a single A record points to your server and a wildcard CNAME record points to your A record: + +``` +A jupyter.example.org 192.168.1.123 +CNAME *.jupyter.example.org jupyter.example.org +``` + +Since this spreads the service across multiple domains, you will likely need wildcard SSL as well, +matching `*.jupyter.example.org`. + +Unfortunately, for many institutional domains, wildcard DNS and SSL may not be available. + +We also **strongly encourage** serving JupyterHub and user content on a domain that is _not_ a subdomain of any sensitive content. +For reasoning, see [GitHub's discussion of moving user content to github.io from \*.github.com](https://github.blog/2013-04-09-yummy-cookies-across-domains/). + +**If you do plan to serve untrusted users, enabling subdomains is highly encouraged**, +as it resolves many security issues, which are difficult to unavoidable when JupyterHub is on a single-domain. + +:::{important} +JupyterHub makes no guarantees about protecting users from each other unless subdomains are enabled. + +If you want to protect users from each other, you **_must_** enable per-user domains. +::: ### Disable user config If subdomains are unavailable or undesirable, JupyterHub provides a -configuration option `Spawner.disable_user_config`, which can be set to prevent +configuration option `Spawner.disable_user_config = True`, which can be set to prevent the user-owned configuration files from being loaded. After implementing this option, `PATH`s and package installation are the other things that the admin must enforce. @@ -89,23 +123,24 @@ admin must enforce. For most Spawners, `PATH` is not something users can influence, but it's important that the Spawner should _not_ evaluate shell configuration files prior to launching the server. -### Isolate packages using virtualenv +### Isolate packages in a read-only environment -Package isolation is most easily handled by running the single-user server in -a virtualenv with disabled system-site-packages. The user should not have -permission to install packages into this environment. +The user must not have permission to install packages into the environment where the singleuser-server runs. +On a shared system, package isolation is most easily handled by running the single-user server in +a root-owned virtualenv with disabled system-site-packages. +The user must not have permission to install packages into this environment. The same principle extends to the images used by container-based deployments. -If users can select the images in which their servers run, they can disable all security. +If users can select the images in which their servers run, they can disable all security for their own servers. -It is important to note that the control over the environment only affects the -single-user server, and not the environment(s) in which the user's kernel(s) +It is important to note that the control over the environment is only required for the +single-user server, and not the environment(s) in which the users' kernel(s) may run. Installing additional packages in the kernel environment does not pose additional risk to the web application's security. ### Encrypt internal connections with SSL/TLS -By default, all communications on the server, between the proxy, hub, and single --user notebooks are performed unencrypted. Setting the `internal_ssl` flag in +By default, all communications within JupyterHub—between the proxy, hub, and single +-user notebooks—are performed unencrypted. Setting the `internal_ssl` flag in `jupyterhub_config.py` secures the aforementioned routes. Turning this feature on does require that the enabled `Spawner` can use the certificates generated by the `Hub` (the default `LocalProcessSpawner` can, for instance). @@ -119,6 +154,104 @@ Unix permissions to the communication sockets thereby restricting communication to the socket owner. The `internal_ssl` option will eventually extend to securing the `tcp` sockets as well. +### Mitigating same-origin deployments + +While per-user domains are **required** for robust protection of users from each other, +you can mitigate many (but not all) cross-user issues. +First, it is critical that users cannot modify their server environments, as described above. +Second, it is important that users do not have `access:servers` permission to any server other than their own. + +If users can access each others' servers, additional security measures must be enabled, some of which come with distinct user-experience costs. + +Without the [Same-Origin Policy] (SOP) protecting user servers from each other, +each user server is considered a trusted origin for requests to each other user server (and the Hub itself). +Servers _cannot_ meaningfully distinguish requests originating from other user servers, +because SOP implies a great deal of trust, losing many restrictions applied to cross-origin requests. + +That means pages served from each user server can: + +1. arbitrarily modify the path in the Referer +2. make fully authorized requests with cookies +3. access full page contents served from the hub or other servers via popups + +JupyterHub uses distinct xsrf tokens stored in cookies on each server path to attempt to limit requests across. +This has limitations because not all requests are protected by these XSRF tokens, +and unless additional measures are taken, the XSRF tokens from other user prefixes may be retrieved. + +[Same-Origin Policy]: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy + +For example: + +- `Content-Security-Policy` header must prohibit popups and iframes from the same origin. + The following Content-Security-Policy rules are _insecure_ and readily enable users to access each others' servers: + + - `frame-ancestors: 'self'` + - `frame-ancestors: '*'` + - `sandbox allow-popups` + +- Ideally, pages should use the strictest `Content-Security-Policy: sandbox` available, + but this is not feasible in general for JupyterLab pages, which need at least `sandbox allow-same-origin allow-scripts` to work. + +The default Content-Security-Policy for single-user servers is + +``` +frame-ancestors: 'none' +``` + +which prohibits iframe embedding, but not pop-ups. + +A more secure Content-Security-Policy that has some costs to user experience is: + +``` +frame-ancestors: 'none'; sandbox allow-same-origin allow-scripts +``` + +`allow-popups` is not disabled by default because disabling it breaks legitimate functionality, like "Open this in a new tab", and the "JupyterHub Control Panel" menu item. +To reiterate, the right way to avoid these issues is to enable per-user domains, where none of these concerns come up. + +Note: even this level of protection requires administrators maintaining full control over the user server environment. +If users can modify their server environment, these methods are ineffective, as users can readily disable them. + +### Cookie tossing + +Cookie tossing is a technique where another server on a subdomain or peer subdomain can set a cookie +which will be read on another domain. +This is not relevant unless there are other user-controlled servers on a peer domain. + +"Domain-locked" cookies avoid this issue, but have their own restrictions: + +- JupyterHub must be served over HTTPS +- All secure cookies must be set on `/`, not on sub-paths, which means they are shared by all JupyterHub components in a single-domain deployment. + +As a result, this option is only recommended when per-user subdomains are enabled, +to prevent sending all jupyterhub cookies to all user servers. + +To enable domain-locked cookies, set: + +```python +c.JupyterHub.cookie_host_prefix_enabled = True +``` + +```{versionadded} 4.1 + +``` + +### Forced-login + +Jupyter servers can share links with `?token=...`. +JupyterHub prior to 5.0 will accept this request and persist the token for future requests. +This is useful for enabling admins to create 'fully authenticated' links bypassing login. +However, it also means users can share their own links that will log other users into their own servers, +enabling them to serve each other notebooks and other arbitrary HTML, depending on server configuration. + +```{versionadded} 4.1 +Setting environment variable `JUPYTERHUB_ALLOW_TOKEN_IN_URL=0` in the single-user environment can opt out of accepting token auth in URL parameters. +``` + +```{versionadded} 5.0 +Accepting tokens in URLs is disabled by default, and `JUPYTERHUB_ALLOW_TOKEN_IN_URL=1` environment variable must be set to _allow_ token auth in URL parameters. +``` + ## Security audits We recommend that you do periodic reviews of your deployment's security. It's diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index 809efed1..0ffc2b1e 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -8,6 +8,57 @@ command line for details. ## [Unreleased] +## 4.1 + +### 4.1.0 - 2024-03 + +JupyterHub 4.1 is a security release, fixing [CVE-2024-28233]. +All JupyterHub deployments are encouraged to upgrade, +especially those with other user content on peer domains to JupyterHub. + +As always, JupyterHub deployments are especially encouraged to enable per-user domains if protecting users from each other is a concern. + +For more information on securely deploying JupyterHub, see the [web security documentation](web-security). + +[CVE-2024-28233]: https://github.com/jupyterhub/jupyterhub/security/advisories/GHSA-7r3h-4ph8-w38g + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.0.2...4.1.0)) + +#### Enhancements made + +- Backport PR #4628 on branch 4.x (Include LDAP groups in local spawner gids) [#4735](https://github.com/jupyterhub/jupyterhub/pull/4735) ([@minrk](https://github.com/minrk)) +- Backport PR #4561 on branch 4.x (Improve debugging when waiting for servers) [#4714](https://github.com/jupyterhub/jupyterhub/pull/4714) ([@minrk](https://github.com/minrk)) +- Backport PR #4563 on branch 4.x (only set 'domain' field on session-id cookie) [#4707](https://github.com/jupyterhub/jupyterhub/pull/4707) ([@minrk](https://github.com/minrk)) + +#### Bugs fixed + +- Backport PR #4733 on branch 4.x (Catch ValueError while waiting for server to be reachable) [#4734](https://github.com/jupyterhub/jupyterhub/pull/4734) ([@minrk](https://github.com/minrk)) +- Backport PR #4679 on branch 4.x (Unescape jinja username) [#4705](https://github.com/jupyterhub/jupyterhub/pull/4705) ([@minrk](https://github.com/minrk)) +- Backport PR #4630: avoid setting unused oauth state cookies on API requests [#4697](https://github.com/jupyterhub/jupyterhub/pull/4697) ([@minrk](https://github.com/minrk)) +- Backport PR #4632: simplify, avoid errors in parsing accept headers [#4696](https://github.com/jupyterhub/jupyterhub/pull/4696) ([@minrk](https://github.com/minrk)) +- Backport PR #4677 on branch 4.x (Improve validation, docs for token.expires_in) [#4692](https://github.com/jupyterhub/jupyterhub/pull/4692) ([@minrk](https://github.com/minrk)) +- Backport PR #4570 on branch 4.x (fix mutation of frozenset in scope intersection) [#4691](https://github.com/jupyterhub/jupyterhub/pull/4691) ([@minrk](https://github.com/minrk)) +- Backport PR #4562 on branch 4.x (Use `user.stop` to cleanup spawners that stopped while Hub was down) [#4690](https://github.com/jupyterhub/jupyterhub/pull/4690) ([@minrk](https://github.com/minrk)) +- Backport PR #4542 on branch 4.x (Fix include_stopped_servers in paginated next_url) [#4689](https://github.com/jupyterhub/jupyterhub/pull/4689) ([@minrk](https://github.com/minrk)) +- Backport PR #4651 on branch 4.x (avoid attempting to patch removed IPythonHandler with notebook v7) [#4688](https://github.com/jupyterhub/jupyterhub/pull/4688) ([@minrk](https://github.com/minrk)) +- Backport PR #4560 on branch 4.x (singleuser extension: persist token from ?token=... url in cookie) [#4687](https://github.com/jupyterhub/jupyterhub/pull/4687) ([@minrk](https://github.com/minrk)) + +#### Maintenance and upkeep improvements + +- Backport quay.io publishing [#4698](https://github.com/jupyterhub/jupyterhub/pull/4698) ([@minrk](https://github.com/minrk)) +- Backport PR #4617: try to improve reliability of test_external_proxy [#4695](https://github.com/jupyterhub/jupyterhub/pull/4695) ([@minrk](https://github.com/minrk)) +- Backport PR #4618 on branch 4.x (browser test: wait for token request to finish before reloading) [#4694](https://github.com/jupyterhub/jupyterhub/pull/4694) ([@minrk](https://github.com/minrk)) +- preparing 4.x branch [#4685](https://github.com/jupyterhub/jupyterhub/pull/4685) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2023-08-10&to=2024-03-19&type=c)) + +@Achele ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AAchele+updated%3A2023-08-10..2024-03-19&type=Issues)) | @akashthedeveloper ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aakashthedeveloper+updated%3A2023-08-10..2024-03-19&type=Issues)) | @balajialg ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Abalajialg+updated%3A2023-08-10..2024-03-19&type=Issues)) | @BhavyaT-135 ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3ABhavyaT-135+updated%3A2023-08-10..2024-03-19&type=Issues)) | @blink1073 ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Ablink1073+updated%3A2023-08-10..2024-03-19&type=Issues)) | @consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2023-08-10..2024-03-19&type=Issues)) | @fcollonval ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Afcollonval+updated%3A2023-08-10..2024-03-19&type=Issues)) | @I-Am-D-B ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AI-Am-D-B+updated%3A2023-08-10..2024-03-19&type=Issues)) | @jakirkham ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Ajakirkham+updated%3A2023-08-10..2024-03-19&type=Issues)) | @ktaletsk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aktaletsk+updated%3A2023-08-10..2024-03-19&type=Issues)) | @kzgrzendek ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Akzgrzendek+updated%3A2023-08-10..2024-03-19&type=Issues)) | @lumberbot-app ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Alumberbot-app+updated%3A2023-08-10..2024-03-19&type=Issues)) | @manics ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Amanics+updated%3A2023-08-10..2024-03-19&type=Issues)) | @mbiette ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Ambiette+updated%3A2023-08-10..2024-03-19&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2023-08-10..2024-03-19&type=Issues)) | @rcthomas ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Arcthomas+updated%3A2023-08-10..2024-03-19&type=Issues)) | @ryanlovett ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aryanlovett+updated%3A2023-08-10..2024-03-19&type=Issues)) | @sgaist ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Asgaist+updated%3A2023-08-10..2024-03-19&type=Issues)) | @shubham0473 ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Ashubham0473+updated%3A2023-08-10..2024-03-19&type=Issues)) | @Temidayo32 ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3ATemidayo32+updated%3A2023-08-10..2024-03-19&type=Issues)) | @willingc ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Awillingc+updated%3A2023-08-10..2024-03-19&type=Issues)) | @yuvipanda ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Ayuvipanda+updated%3A2023-08-10..2024-03-19&type=Issues)) + ## 4.0 ### 4.0.2 - 2023-08-10 diff --git a/jupyterhub/_xsrf_utils.py b/jupyterhub/_xsrf_utils.py new file mode 100644 index 00000000..805d7df8 --- /dev/null +++ b/jupyterhub/_xsrf_utils.py @@ -0,0 +1,210 @@ +"""utilities for XSRF + +Extends tornado's xsrf token checks with the following: + +- only set xsrf cookie on navigation requests (cannot be fetched) + +This utility file enables the consistent reuse of these functions +in both Hub and single-user code +""" + +import base64 +import hashlib +from datetime import datetime, timedelta, timezone +from http.cookies import SimpleCookie + +from tornado import web +from tornado.httputil import format_timestamp +from tornado.log import app_log + + +def _get_signed_value_urlsafe(handler, name, b64_value): + """Like get_signed_value (used in get_secure_cookie), but for urlsafe values + + Decodes urlsafe_base64-encoded signed values + + Returns None if any decoding failed + """ + if b64_value is None: + return None + + if isinstance(b64_value, str): + try: + b64_value = b64_value.encode("ascii") + except UnicodeEncodeError: + app_log.warning("Invalid value %r", b64_value) + return None + # re-pad, since we stripped padding in _create_signed_value + remainder = len(b64_value) % 4 + if remainder: + b64_value = b64_value + (b'=' * (4 - remainder)) + try: + value = base64.urlsafe_b64decode(b64_value) + except ValueError: + app_log.warning("Invalid base64 value %r", b64_value) + return None + + return web.decode_signed_value( + handler.application.settings["cookie_secret"], + name, + value, + max_age_days=31, + min_version=2, + ) + + +def _create_signed_value_urlsafe(handler, name, value): + """Like tornado's create_signed_value (used in set_secure_cookie), but returns urlsafe bytes""" + + signed_value = handler.create_signed_value(name, value) + return base64.urlsafe_b64encode(signed_value).rstrip(b"=") + + +def _clear_invalid_xsrf_cookie(handler, cookie_path): + """ + Clear invalid XSRF cookie + + This may an old XSRF token, or one set on / by another application. + Because we cannot trust browsers or tornado to give us the more specific cookie, + try to clear _both_ on / and on our prefix, + then reload the page. + """ + + expired = format_timestamp(datetime.now(timezone.utc) - timedelta(days=366)) + cookie = SimpleCookie() + cookie["_xsrf"] = "" + morsel = cookie["_xsrf"] + morsel["expires"] = expired + morsel["path"] = "/" + # use Set-Cookie directly, + # because tornado's set_cookie and clear_cookie use a single _dict_, + # so we can't clear a cookie on multiple paths and then set it + handler.add_header("Set-Cookie", morsel.OutputString(None)) + if cookie_path != "/": + # clear it multiple times! + morsel["path"] = cookie_path + handler.add_header("Set-Cookie", morsel.OutputString(None)) + + if ( + handler.request.method.lower() == "get" + and handler.request.headers.get("Sec-Fetch-Mode", "navigate") == "navigate" + ): + # reload current page because any subsequent set_cookie + # will cancel the clearing of the cookie + # this only makes sense on GET requests + handler.redirect(handler.request.uri) + # halt any other processing of the request + raise web.Finish() + + +def get_xsrf_token(handler, cookie_path=""): + """Override tornado's xsrf token to add further restrictions + + - only set cookie for regular pages (not API requests) + - include login info in xsrf token + - verify signature + """ + # original: https://github.com/tornadoweb/tornado/blob/v6.4.0/tornado/web.py#L1455 + if hasattr(handler, "_xsrf_token"): + return handler._xsrf_token + + _set_cookie = False + # the raw cookie is the token + xsrf_token = xsrf_cookie = handler.get_cookie("_xsrf") + if xsrf_token: + try: + xsrf_token = xsrf_token.encode("ascii") + except UnicodeEncodeError: + xsrf_token = None + + xsrf_id_cookie = _get_signed_value_urlsafe(handler, "_xsrf", xsrf_token) + if xsrf_cookie and not xsrf_id_cookie: + # we have a cookie, but it's invalid! + # handle possibility of _xsrf being set multiple times, + # e.g. on / and on /hub/ + # this will reload the page if it's a GET request + app_log.warning( + "Attempting to clear invalid _xsrf cookie %r", xsrf_cookie[:4] + "..." + ) + _clear_invalid_xsrf_cookie(handler, cookie_path) + + # check the decoded, signed value for validity + xsrf_id = handler._xsrf_token_id + if xsrf_id_cookie != xsrf_id: + # this will usually happen on the first page request after login, + # which changes the inputs to the token id + if xsrf_id_cookie: + app_log.debug("xsrf id mismatch %r != %r", xsrf_id_cookie, xsrf_id) + # generate new value + xsrf_token = _create_signed_value_urlsafe(handler, "_xsrf", xsrf_id) + # only set cookie on regular navigation pages + # i.e. not API requests, etc. + # insecure URLs (public hostname/ip, no https) + # don't set Sec-Fetch-Mode. + # consequence of assuming 'navigate': setting a cookie unnecessarily + # consequence of assuming not 'navigate': xsrf never set, nothing works + _set_cookie = ( + handler.request.headers.get("Sec-Fetch-Mode", "navigate") == "navigate" + ) + + if _set_cookie: + xsrf_cookie_kwargs = {} + xsrf_cookie_kwargs.update(handler.settings.get('xsrf_cookie_kwargs', {})) + xsrf_cookie_kwargs.setdefault("path", cookie_path) + if not handler.current_user: + # limit anonymous xsrf cookies to one hour + xsrf_cookie_kwargs.pop("expires", None) + xsrf_cookie_kwargs.pop("expires_days", None) + xsrf_cookie_kwargs["max_age"] = 3600 + app_log.info( + "Setting new xsrf cookie for %r %r", + xsrf_id, + xsrf_cookie_kwargs, + ) + handler.set_cookie("_xsrf", xsrf_token, **xsrf_cookie_kwargs) + handler._xsrf_token = xsrf_token + return xsrf_token + + +def check_xsrf_cookie(handler): + """Check that xsrf cookie matches xsrf token in request""" + # overrides tornado's implementation + # because we changed what a correct value should be in xsrf_token + + token = ( + handler.get_argument("_xsrf", None) + or handler.request.headers.get("X-Xsrftoken") + or handler.request.headers.get("X-Csrftoken") + ) + + if not token: + raise web.HTTPError( + 403, f"'_xsrf' argument missing from {handler.request.method}" + ) + + try: + token = token.encode("utf8") + except UnicodeEncodeError: + raise web.HTTPError(403, "'_xsrf' argument invalid") + + if token != handler.xsrf_token: + raise web.HTTPError( + 403, f"XSRF cookie does not match {handler.request.method.upper()} argument" + ) + + +def _anonymous_xsrf_id(handler): + """Generate an appropriate xsrf token id for an anonymous request + + Currently uses hash of request ip and user-agent + + These are typically used only for the initial login page, + so only need to be valid for a few seconds to a few minutes + (enough to submit a login form with MFA). + """ + hasher = hashlib.sha256() + hasher.update(handler.request.remote_ip.encode("ascii")) + hasher.update( + handler.request.headers.get("User-Agent", "").encode("utf8", "replace") + ) + return base64.urlsafe_b64encode(hasher.digest()).decode("ascii") diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index d0e20576..4eb784b5 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -76,15 +76,8 @@ class APIHandler(BaseHandler): return True - async def prepare(self): - await super().prepare() - # tornado only checks xsrf on non-GET - # we also check xsrf on GETs to API endpoints - # make sure this runs after auth, which happens in super().prepare() - if self.request.method not in {"HEAD", "OPTIONS"} and self.settings.get( - "xsrf_cookies" - ): - self.check_xsrf_cookie() + # we also check xsrf on GETs to API endpoints + _xsrf_safe_methods = {"HEAD", "OPTIONS"} def check_xsrf_cookie(self): if not hasattr(self, '_jupyterhub_user'): diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 01380b42..7d6875a6 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -402,6 +402,25 @@ class JupyterHub(Application): Useful for daemonizing JupyterHub. """, ).tag(config=True) + + cookie_host_prefix_enabled = Bool( + False, + help="""Enable `__Host-` prefix on authentication cookies. + + The `__Host-` prefix on JupyterHub cookies provides further + protection against cookie tossing when untrusted servers + may control subdomains of your jupyterhub deployment. + + _However_, it also requires that cookies be set on the path `/`, + which means they are shared by all JupyterHub components, + so a compromised server component will have access to _all_ JupyterHub-related + cookies of the visiting browser. + It is recommended to only combine `__Host-` cookies with per-user domains. + + .. versionadded:: 4.1 + """, + ).tag(config=True) + cookie_max_age_days = Float( 14, help="""Number of days for a login cookie to be valid. @@ -2034,6 +2053,8 @@ class JupyterHub(Application): hub_args['port'] = self.hub_port self.hub = Hub(**hub_args) + if self.cookie_host_prefix_enabled: + self.hub.cookie_name = "__Host-" + self.hub.cookie_name if not self.subdomain_host: api_prefix = url_path_join(self.hub.base_url, "api/") @@ -3051,6 +3072,7 @@ class JupyterHub(Application): default_url=self.default_url, public_url=urlparse(self.public_url) if self.public_url else "", cookie_secret=self.cookie_secret, + cookie_host_prefix_enabled=self.cookie_host_prefix_enabled, cookie_max_age_days=self.cookie_max_age_days, redirect_to_server=self.redirect_to_server, login_url=login_url, diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 9c426ce3..872f1d3a 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -24,6 +24,7 @@ from tornado.log import app_log from tornado.web import RequestHandler, addslash from .. import __version__, orm, roles, scopes +from .._xsrf_utils import _anonymous_xsrf_id, check_xsrf_cookie, get_xsrf_token from ..metrics import ( PROXY_ADD_DURATION_SECONDS, PROXY_DELETE_DURATION_SECONDS, @@ -100,7 +101,14 @@ class BaseHandler(RequestHandler): self.log.error("Rolling back session due to database error") self.db.rollback() self._resolve_roles_and_scopes() - return await maybe_future(super().prepare()) + await maybe_future(super().prepare()) + # run xsrf check after prepare + # because our version takes auth info into account + if ( + self.request.method not in self._xsrf_safe_methods + and self.application.settings.get("xsrf_cookies") + ): + self.check_xsrf_cookie() @property def log(self): @@ -205,9 +213,13 @@ class BaseHandler(RequestHandler): """The default Content-Security-Policy header Can be overridden by defining Content-Security-Policy in settings['headers'] + + ..versionchanged:: 4.1 + + Change default frame-ancestors from 'self' to 'none' """ return '; '.join( - ["frame-ancestors 'self'", "report-uri " + self.csp_report_uri] + ["frame-ancestors 'none'", "report-uri " + self.csp_report_uri] ) def get_content_type(self): @@ -217,7 +229,6 @@ class BaseHandler(RequestHandler): """ Set any headers passed as tornado_settings['headers']. - By default sets Content-Security-Policy of frame-ancestors 'self'. Also responsible for setting content-type header """ # wrap in HTTPHeaders for case-insensitivity @@ -239,17 +250,63 @@ class BaseHandler(RequestHandler): # Login and cookie-related # --------------------------------------------------------------- + _xsrf_safe_methods = {"GET", "HEAD", "OPTIONS"} + + @property + def _xsrf_token_id(self): + """Value to be signed/encrypted for xsrf token + + include login info in xsrf token + this means xsrf tokens are tied to logged-in users, + and change after a user logs in. + + While the user is not yet logged in, + an anonymous value is used, to prevent portability. + These anonymous values are short-lived. + """ + # cases: + # 1. logged in, session id (session_id:user_id) + # 2. logged in, no session id (anonymous_id:user_id) + # 3. not logged in, session id (session_id:anonymous_id) + # 4. no cookies at all, use single anonymous value (:anonymous_id) + session_id = self.get_session_cookie() + if self.current_user: + if isinstance(self.current_user, User): + user_id = self.current_user.cookie_id + else: + # this shouldn't happen, but may if e.g. a Service attempts to fetch a page, + # which usually won't work, but this method should not be what raises + user_id = "" + if not session_id: + # no session id, use non-portable anonymous id + session_id = _anonymous_xsrf_id(self) + else: + # not logged in yet, use non-portable anonymous id + user_id = _anonymous_xsrf_id(self) + xsrf_id = f"{session_id}:{user_id}".encode("utf8", "replace") + return xsrf_id + + @property + def xsrf_token(self): + """Override tornado's xsrf token with further restrictions + + - only set cookie for regular pages + - include login info in xsrf token + - verify signature + """ + return get_xsrf_token(self, cookie_path=self.hub.base_url) + def check_xsrf_cookie(self): - try: - return super().check_xsrf_cookie() - except web.HTTPError as e: - # ensure _jupyterhub_user is defined on rejected requests - if not hasattr(self, "_jupyterhub_user"): - self._jupyterhub_user = None - self._resolve_roles_and_scopes() - # rewrite message because we use this on methods other than POST - e.log_message = e.log_message.replace("POST", self.request.method) - raise + """Check that xsrf cookie matches xsrf token in request""" + # overrides tornado's implementation + # because we changed what a correct value should be in xsrf_token + + if not hasattr(self, "_jupyterhub_user"): + # run too early to check the value + # tornado runs this before 'prepare', + # but we run it again after so auth info is available, which happens in 'prepare' + return None + return check_xsrf_cookie(self) @property def admin_users(self): @@ -526,6 +583,16 @@ class BaseHandler(RequestHandler): user = self._user_from_orm(u) return user + def clear_cookie(self, cookie_name, **kwargs): + """Clear a cookie + + overrides RequestHandler to always handle __Host- prefix correctly + """ + if cookie_name.startswith("__Host-"): + kwargs["path"] = "/" + kwargs["secure"] = True + return super().clear_cookie(cookie_name, **kwargs) + def clear_login_cookie(self, name=None): kwargs = {} user = self.get_current_user_cookie() @@ -597,6 +664,11 @@ class BaseHandler(RequestHandler): kwargs.update(self.settings.get('cookie_options', {})) kwargs.update(overrides) + if key.startswith("__Host-"): + # __Host- cookies must be secure and on / + kwargs["path"] = "/" + kwargs["secure"] = True + if encrypted: set_cookie = self.set_secure_cookie else: @@ -626,7 +698,9 @@ class BaseHandler(RequestHandler): Session id cookie is *not* encrypted, so other services on this domain can read it. """ - session_id = uuid.uuid4().hex + if not hasattr(self, "_session_id"): + self._session_id = uuid.uuid4().hex + session_id = self._session_id # if using subdomains, set session cookie on the domain, # which allows it to be shared by subdomains. # if domain is unspecified, it is _more_ restricted to only the setting domain diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index 861af988..d974dcdc 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -35,6 +35,7 @@ import socket import string import time import warnings +from functools import partial from http import HTTPStatus from unittest import mock from urllib.parse import urlencode, urlparse @@ -45,6 +46,7 @@ from tornado.log import app_log from tornado.web import HTTPError, RequestHandler from traitlets import ( Any, + Bool, Dict, Instance, Integer, @@ -56,8 +58,9 @@ from traitlets import ( ) from traitlets.config import SingletonConfigurable +from .._xsrf_utils import _anonymous_xsrf_id, check_xsrf_cookie, get_xsrf_token from ..scopes import _intersect_expanded_scopes -from ..utils import get_browser_protocol, url_path_join +from ..utils import _bool_env, get_browser_protocol, url_path_join def check_scopes(required_scopes, scopes): @@ -356,6 +359,46 @@ class HubAuth(SingletonConfigurable): """, ).tag(config=True) + allow_token_in_url = Bool( + _bool_env("JUPYTERHUB_ALLOW_TOKEN_IN_URL", default=True), + help="""Allow requests to pages with ?token=... in the URL + + This allows starting a user session by sharing a URL with credentials, + bypassing authentication with the Hub. + + If False, tokens in URLs will be ignored by the server, + except on websocket requests. + + Has no effect on websocket requests, + which can only reliably authenticate via token in the URL, + as recommended by browser Websocket implementations. + + This will default to False in JupyterHub 5. + + .. versionadded:: 4.1 + + .. versionchanged:: 5.0 + default changed to False + """, + ).tag(config=True) + + allow_websocket_cookie_auth = Bool( + _bool_env("JUPYTERHUB_ALLOW_WEBSOCKET_COOKIE_AUTH", default=True), + help="""Allow websocket requests with only cookie for authentication + + Cookie-authenticated websockets cannot be protected from other user servers unless per-user domains are used. + Disabling cookie auth on websockets protects user servers from each other, + but may break some user applications. + Per-user domains eliminate the need to lock this down. + + JupyterLab 4.1.2 and Notebook 6.5.6, 7.1.0 will not work + because they rely on cookie authentication without + API or XSRF tokens. + + .. versionadded:: 4.1 + """, + ).tag(config=True) + cookie_options = Dict( help="""Additional options to pass when setting cookies. @@ -374,6 +417,40 @@ class HubAuth(SingletonConfigurable): else: return {} + cookie_host_prefix_enabled = Bool( + False, + help="""Enable `__Host-` prefix on authentication cookies. + + The `__Host-` prefix on JupyterHub cookies provides further + protection against cookie tossing when untrusted servers + may control subdomains of your jupyterhub deployment. + + _However_, it also requires that cookies be set on the path `/`, + which means they are shared by all JupyterHub components, + so a compromised server component will have access to _all_ JupyterHub-related + cookies of the visiting browser. + It is recommended to only combine `__Host-` cookies with per-user domains. + + Set via $JUPYTERHUB_COOKIE_HOST_PREFIX_ENABLED + """, + ).tag(config=True) + + @default("cookie_host_prefix_enabled") + def _default_cookie_host_prefix_enabled(self): + return _bool_env("JUPYTERHUB_COOKIE_HOST_PREFIX_ENABLED") + + @property + def cookie_path(self): + """ + Path prefix on which to set cookies + + self.base_url, but '/' when cookie_host_prefix_enabled is True + """ + if self.cookie_host_prefix_enabled: + return "/" + else: + return self.base_url + cookie_cache_max_age = Integer(help="DEPRECATED. Use cache_max_age") @observe('cookie_cache_max_age') @@ -636,6 +713,17 @@ class HubAuth(SingletonConfigurable): auth_header_name = 'Authorization' auth_header_pat = re.compile(r'(?:token|bearer)\s+(.+)', re.IGNORECASE) + def _get_token_url(self, handler): + """Get the token from the URL + + Always run for websockets, + otherwise run only if self.allow_token_in_url + """ + fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unspecified") + if self.allow_token_in_url or fetch_mode == "websocket": + return handler.get_argument("token", "") + return "" + def get_token(self, handler, in_cookie=True): """Get the token authenticating a request @@ -651,8 +739,7 @@ class HubAuth(SingletonConfigurable): Args: handler (tornado.web.RequestHandler): the current request handler """ - - user_token = handler.get_argument('token', '') + user_token = self._get_token_url(handler) if not user_token: # get it from Authorization header m = self.auth_header_pat.match( @@ -702,6 +789,14 @@ class HubAuth(SingletonConfigurable): """ return self._call_coroutine(sync, self._get_user, handler) + def _patch_xsrf(self, handler): + """Overridden in HubOAuth + + HubAuth base class doesn't handle xsrf, + which is only relevant for cookie-based auth + """ + return + async def _get_user(self, handler): # only allow this to be called once per handler # avoids issues if an error is raised, @@ -709,6 +804,9 @@ class HubAuth(SingletonConfigurable): if hasattr(handler, '_cached_hub_user'): return handler._cached_hub_user + # patch XSRF checks, which will apply after user check + self._patch_xsrf(handler) + handler._cached_hub_user = user_model = None session_id = self.get_session_id(handler) @@ -794,7 +892,10 @@ class HubOAuth(HubAuth): because we don't want to use the same cookie name across OAuth clients. """ - return self.oauth_client_id + cookie_name = self.oauth_client_id + if self.cookie_host_prefix_enabled: + cookie_name = "__Host-" + cookie_name + return cookie_name @property def state_cookie_name(self): @@ -806,22 +907,103 @@ class HubOAuth(HubAuth): def _get_token_cookie(self, handler): """Base class doesn't store tokens in cookies""" + + fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unset") + if fetch_mode == "websocket" and not self.allow_websocket_cookie_auth: + # disallow cookie auth on websockets + return None + token = handler.get_secure_cookie(self.cookie_name) if token: # decode cookie bytes token = token.decode('ascii', 'replace') return token - async def _get_user_cookie(self, handler): + def _get_xsrf_token_id(self, handler): + """Get contents for xsrf token for a given Handler + + This is the value to be encrypted & signed in the xsrf token + """ token = self._get_token_cookie(handler) session_id = self.get_session_id(handler) + if token: + token_hash = hashlib.sha256(token.encode("ascii", "replace")).hexdigest() + if not session_id: + session_id = _anonymous_xsrf_id(handler) + else: + token_hash = _anonymous_xsrf_id(handler) + return f"{session_id}:{token_hash}".encode("ascii", "replace") + + def _patch_xsrf(self, handler): + """Patch handler to inject JuptyerHub xsrf token behavior""" + handler._xsrf_token_id = self._get_xsrf_token_id(handler) + # override xsrf_token property on class, + # so it's still a getter, not invoked immediately + handler.__class__.xsrf_token = property( + partial(get_xsrf_token, cookie_path=self.base_url) + ) + handler.check_xsrf_cookie = partial(self.check_xsrf_cookie, handler) + + def check_xsrf_cookie(self, handler): + """check_xsrf_cookie patch + + Applies JupyterHub check_xsrf_cookie if not token authenticated + """ + if getattr(handler, '_token_authenticated', False): + return + check_xsrf_cookie(handler) + + def _clear_cookie(self, handler, cookie_name, **kwargs): + """Clear a cookie, handling __Host- prefix""" + # Set-Cookie is rejected without 'secure', + # this includes clearing cookies! + if cookie_name.startswith("__Host-"): + kwargs["path"] = "/" + kwargs["secure"] = True + return handler.clear_cookie(cookie_name, **kwargs) + + def _needs_check_xsrf(self, handler): + """Does the given cookie-authenticated request need to check xsrf?""" + if getattr(handler, "_token_authenticated", False): + return False + + fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unspecified") + if fetch_mode in {"websocket", "no-cors"} or ( + fetch_mode in {"navigate", "unspecified"} + and handler.request.method.lower() in {"get", "head", "options"} + ): + # no xsrf check needed for regular page views or no-cors + # or websockets after allow_websocket_cookie_auth passes + if fetch_mode == "unspecified": + self.log.warning( + f"Skipping XSRF check for insecure request {handler.request.method} {handler.request.path}" + ) + return False + else: + return True + + async def _get_user_cookie(self, handler): + # check xsrf if needed + token = self._get_token_cookie(handler) + session_id = self.get_session_id(handler) + if token and self._needs_check_xsrf(handler): + try: + self.check_xsrf_cookie(handler) + except HTTPError as e: + self.log.error( + f"Not accepting cookie auth on {handler.request.method} {handler.request.path}: {e}" + ) + # don't proceed with cookie auth unless xsrf is okay + # don't raise either, because that makes a mess + return None + if token: user_model = await self.user_for_token( token, session_id=session_id, sync=False ) if user_model is None: app_log.warning("Token stored in cookie may have expired") - handler.clear_cookie(self.cookie_name) + self._clear_cookie(handler, self.cookie_name, path=self.cookie_path) return user_model # HubOAuth API @@ -962,7 +1144,7 @@ class HubOAuth(HubAuth): cookie_name = self.state_cookie_name state_id = self.generate_state(next_url, **extra_state) kwargs = { - 'path': self.base_url, + 'path': self.cookie_path, 'httponly': True, # Expire oauth state cookie in ten minutes. # Usually this will be cleared by completed login @@ -1020,9 +1202,9 @@ class HubOAuth(HubAuth): """Clear persisted oauth state""" for cookie_name, cookie in handler.request.cookies.items(): if cookie_name.startswith(self.state_cookie_name): - handler.clear_cookie( + self._clear_cookie( cookie_name, - path=self.base_url, + path=self.cookie_path, ) def _decode_state(self, state_id, /): @@ -1044,8 +1226,11 @@ class HubOAuth(HubAuth): def set_cookie(self, handler, access_token): """Set a cookie recording OAuth result""" - kwargs = {'path': self.base_url, 'httponly': True} - if get_browser_protocol(handler.request) == 'https': + kwargs = {'path': self.cookie_path, 'httponly': True} + if ( + get_browser_protocol(handler.request) == 'https' + or self.cookie_host_prefix_enabled + ): kwargs['secure'] = True # load user cookie overrides kwargs.update(self.cookie_options) @@ -1063,7 +1248,7 @@ class HubOAuth(HubAuth): Args: handler (tornado.web.RequestHandler): the current request handler """ - handler.clear_cookie(self.cookie_name, path=self.base_url) + self._clear_cookie(handler, self.cookie_name, path=self.cookie_path) class UserNotAllowed(Exception): @@ -1275,7 +1460,7 @@ class HubAuthenticated: return try: self._hub_auth_user_cache = self.check_hub_user(user_model) - except UserNotAllowed as e: + except UserNotAllowed: # cache None, in case get_user is called again while processing the error self._hub_auth_user_cache = None @@ -1297,6 +1482,25 @@ class HubAuthenticated: self.hub_auth._persist_url_token_if_set(self) return self._hub_auth_user_cache + @property + def _xsrf_token_id(self): + if hasattr(self, "__xsrf_token_id"): + return self.__xsrf_token_id + if not isinstance(self.hub_auth, HubOAuth): + return "" + return self.hub_auth._get_xsrf_token_id(self) + + @_xsrf_token_id.setter + def _xsrf_token_id(self, value): + self.__xsrf_token_id = value + + @property + def xsrf_token(self): + return get_xsrf_token(self, cookie_path=self.hub_auth.base_url) + + def check_xsrf_cookie(self): + return self.hub_auth.check_xsrf_cookie(self) + class HubOAuthenticated(HubAuthenticated): """Simple subclass of HubAuthenticated using OAuth instead of old shared cookies""" @@ -1332,7 +1536,7 @@ class HubOAuthCallbackHandler(HubOAuthenticated, RequestHandler): cookie_state = self.get_secure_cookie(cookie_name) # clear cookie state now that we've consumed it if cookie_state: - self.clear_cookie(cookie_name, path=self.hub_auth.base_url) + self.hub_auth.clear_oauth_state_cookies(self) else: # completing oauth with stale state, but already logged in. # stop here and redirect to default URL @@ -1349,8 +1553,13 @@ class HubOAuthCallbackHandler(HubOAuthenticated, RequestHandler): # check that state matches if arg_state != cookie_state: - app_log.warning("oauth state %r != %r", arg_state, cookie_state) - raise HTTPError(403, "OAuth state does not match. Try logging in again.") + app_log.warning( + "oauth state argument %r != cookie %s=%r", + arg_state, + cookie_name, + cookie_state, + ) + raise HTTPError(403, "oauth state does not match. Try logging in again.") next_url = self.hub_auth.get_next_url(cookie_state) # clear consumed state from _oauth_states cache now that we're done with it self.hub_auth.clear_oauth_state(cookie_state) diff --git a/jupyterhub/singleuser/extension.py b/jupyterhub/singleuser/extension.py index 4fe2d1dd..dc9e5f36 100644 --- a/jupyterhub/singleuser/extension.py +++ b/jupyterhub/singleuser/extension.py @@ -44,6 +44,7 @@ from jupyterhub._version import __version__, _check_version from jupyterhub.log import log_request from jupyterhub.services.auth import HubOAuth, HubOAuthCallbackHandler from jupyterhub.utils import ( + _bool_env, exponential_backoff, isoformat, make_ssl_context, @@ -55,17 +56,6 @@ from ._disable_user_config import _disable_user_config SINGLEUSER_TEMPLATES_DIR = str(Path(__file__).parent.resolve().joinpath("templates")) -def _bool_env(key): - """Cast an environment variable to bool - - 0, empty, or unset is False; All other values are True. - """ - if os.environ.get(key, "") in {"", "0"}: - return False - else: - return True - - def _exclude_home(path_list): """Filter out any entries in a path list that are in my home directory. @@ -127,6 +117,9 @@ class JupyterHubIdentityProvider(IdentityProvider): # HubAuth gets most of its config from the environment return HubOAuth(parent=self) + def _patch_xsrf(self, handler): + self.hub_auth._patch_xsrf(handler) + def _patch_get_login_url(self, handler): original_get_login_url = handler.get_login_url @@ -161,6 +154,7 @@ class JupyterHubIdentityProvider(IdentityProvider): if hasattr(handler, "_jupyterhub_user"): return handler._jupyterhub_user self._patch_get_login_url(handler) + self._patch_xsrf(handler) user = await self.hub_auth.get_user(handler, sync=False) if user is None: handler._jupyterhub_user = None @@ -632,6 +626,9 @@ class JupyterHubSingleUser(ExtensionApp): app.web_app.settings["page_config_hook"] = ( app.identity_provider.page_config_hook ) + # disable xsrf_cookie checks by Tornado, which run too early + # checks in Jupyter Server are unconditional + app.web_app.settings["xsrf_cookies"] = False # if the user has configured a log function in the tornado settings, do not override it if not 'log_function' in app.config.ServerApp.get('tornado_settings', {}): app.web_app.settings["log_function"] = log_request @@ -642,6 +639,9 @@ class JupyterHubSingleUser(ExtensionApp): # check jupyterhub version app.io_loop.run_sync(self.check_hub_version) + # set default CSP to prevent iframe embedding across jupyterhub components + headers.setdefault("Content-Security-Policy", "frame-ancestors 'none'") + async def _start_activity(): self._activity_task = asyncio.ensure_future(self.keep_activity_updated()) diff --git a/jupyterhub/singleuser/mixins.py b/jupyterhub/singleuser/mixins.py index 83b34aed..c08fda9d 100755 --- a/jupyterhub/singleuser/mixins.py +++ b/jupyterhub/singleuser/mixins.py @@ -45,21 +45,15 @@ from traitlets.config import Configurable from .._version import __version__, _check_version from ..log import log_request from ..services.auth import HubOAuth, HubOAuthCallbackHandler, HubOAuthenticated -from ..utils import exponential_backoff, isoformat, make_ssl_context, url_path_join +from ..utils import ( + _bool_env, + exponential_backoff, + isoformat, + make_ssl_context, + url_path_join, +) from ._disable_user_config import _disable_user_config, _exclude_home - -def _bool_env(key): - """Cast an environment variable to bool - - 0, empty, or unset is False; All other values are True. - """ - if os.environ.get(key, "") in {"", "0"}: - return False - else: - return True - - # Authenticate requests with the Hub @@ -683,10 +677,10 @@ class SingleUserNotebookAppMixin(Configurable): ) headers = s.setdefault('headers', {}) headers['X-JupyterHub-Version'] = __version__ - # set CSP header directly to workaround bugs in jupyter/notebook 5.0 + # set default CSP to prevent iframe embedding across jupyterhub components headers.setdefault( 'Content-Security-Policy', - ';'.join(["frame-ancestors 'self'", "report-uri " + csp_report_uri]), + ';'.join(["frame-ancestors 'none'", "report-uri " + csp_report_uri]), ) super().init_webapp() diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 6c0306d2..392d67e8 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -163,6 +163,7 @@ class Spawner(LoggingConfigurable): hub = Any() orm_spawner = Any() cookie_options = Dict() + cookie_host_prefix_enabled = Bool() public_url = Unicode(help="Public URL of this spawner's server") public_hub_url = Unicode(help="Public URL of the Hub itself") @@ -1006,6 +1007,10 @@ class Spawner(LoggingConfigurable): env['JUPYTERHUB_CLIENT_ID'] = self.oauth_client_id if self.cookie_options: env['JUPYTERHUB_COOKIE_OPTIONS'] = json.dumps(self.cookie_options) + + env["JUPYTERHUB_COOKIE_HOST_PREFIX_ENABLED"] = str( + int(self.cookie_host_prefix_enabled) + ) env['JUPYTERHUB_HOST'] = self.hub.public_host env['JUPYTERHUB_OAUTH_CALLBACK_URL'] = url_path_join( self.user.url, url_escape_path(self.name), 'oauth_callback' diff --git a/jupyterhub/tests/browser/test_browser.py b/jupyterhub/tests/browser/test_browser.py index 9054138a..95a53952 100644 --- a/jupyterhub/tests/browser/test_browser.py +++ b/jupyterhub/tests/browser/test_browser.py @@ -1,6 +1,8 @@ """Tests for the Playwright Python""" +import asyncio import json +import pprint import re from unittest import mock from urllib.parse import parse_qs, urlparse @@ -11,7 +13,7 @@ from tornado.escape import url_escape from tornado.httputil import url_concat from jupyterhub import orm, roles, scopes -from jupyterhub.tests.utils import public_host, public_url, ujoin +from jupyterhub.tests.utils import async_requests, public_host, public_url, ujoin from jupyterhub.utils import url_escape_path, url_path_join pytestmark = pytest.mark.browser @@ -44,7 +46,7 @@ async def test_submit_login_form(app, browser, user_special_chars): login_url = url_path_join(public_host(app), app.hub.base_url, "login") await browser.goto(login_url) await login(browser, user.name, password=user.name) - expected_url = ujoin(public_url(app), f"/user/{user_special_chars.urlname}/") + expected_url = public_url(app, user) await expect(browser).to_have_url(expected_url) @@ -56,7 +58,7 @@ async def test_submit_login_form(app, browser, user_special_chars): # will encode given parameters for an unauthenticated URL in the next url # the next parameter will contain the app base URL (replaces BASE_URL in tests) 'spawn', - [('param', 'value')], + {'param': 'value'}, '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', ), @@ -64,15 +66,15 @@ async def test_submit_login_form(app, browser, user_special_chars): # login?param=fromlogin&next=encoded(/hub/spawn?param=value) # will drop parameters given to the login page, passing only the next url 'login', - [('param', 'fromlogin'), ('next', '/hub/spawn?param=value')], - '/hub/login?param=fromlogin&next=%2Fhub%2Fspawn%3Fparam%3Dvalue', - '/hub/login?next=%2Fhub%2Fspawn%3Fparam%3Dvalue', + {'param': 'fromlogin', 'next': '/hub/spawn?param=value'}, + '/hub/login?param=fromlogin&next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', + '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', ), ( # login?param=value&anotherparam=anothervalue # will drop parameters given to the login page, and use an empty next url 'login', - [('param', 'value'), ('anotherparam', 'anothervalue')], + {'param': 'value', 'anotherparam': 'anothervalue'}, '/hub/login?param=value&anotherparam=anothervalue', '/hub/login?next=', ), @@ -80,7 +82,7 @@ async def test_submit_login_form(app, browser, user_special_chars): # login # simplest case, accessing the login URL, gives an empty next url 'login', - [], + {}, '/hub/login', '/hub/login?next=', ), @@ -98,6 +100,8 @@ async def test_open_url_login( user = user_special_chars.user login_url = url_path_join(public_host(app), app.hub.base_url, url) await browser.goto(login_url) + if params.get("next"): + params["next"] = url_path_join(app.base_url, params["next"]) url_new = url_path_join(public_host(app), app.hub.base_url, url_concat(url, params)) print(url_new) await browser.goto(url_new) @@ -853,12 +857,15 @@ async def test_oauth_page( oauth_client.allowed_scopes = sorted(roles.roles_to_scopes([service_role])) app.db.commit() # open the service url in the browser - service_url = url_path_join(public_url(app, service) + 'owhoami/?arg=x') + service_url = url_path_join(public_url(app, service), 'owhoami/?arg=x') await browser.goto(service_url) - expected_redirect_url = url_path_join( - app.base_url + f"services/{service.name}/oauth_callback" - ) + if app.subdomain_host: + expected_redirect_url = url_path_join( + public_url(app, service), "oauth_callback" + ) + else: + expected_redirect_url = url_path_join(service.prefix, "oauth_callback") expected_client_id = f"service-{service.name}" # decode the URL @@ -1236,3 +1243,225 @@ async def test_start_stop_server_on_admin_page( await expect(browser.get_by_role("button", name="Spawn Page")).to_have_count( len(users_list) ) + + +@pytest.mark.parametrize( + "case", + [ + "fresh", + "invalid", + "valid-prefix-invalid-root", + ], +) +async def test_login_xsrf_initial_cookies(app, browser, case, username): + """Test that login works with various initial states for xsrf tokens + + Page will be reloaded with correct values + """ + hub_root = public_host(app) + hub_url = url_path_join(public_host(app), app.hub.base_url) + login_url = url_path_join( + hub_url, url_concat("login", {"next": url_path_join(app.base_url, "/hub/home")}) + ) + # start with all cookies cleared + await browser.context.clear_cookies() + if case == "invalid": + await browser.context.add_cookies( + [{"name": "_xsrf", "value": "invalid-hub-prefix", "url": hub_url}] + ) + elif case == "valid-prefix-invalid-root": + await browser.goto(login_url) + # first visit sets valid xsrf cookie + cookies = await browser.context.cookies() + assert len(cookies) == 1 + # second visit is also made with invalid xsrf on `/` + # handling of this behavior is undefined in HTTP itself! + # _either_ the invalid cookie on / is ignored + # _or_ both will be cleared + # currently, this test assumes the observed behavior, + # which is that the invalid cookie on `/` has _higher_ priority + await browser.context.add_cookies( + [{"name": "_xsrf", "value": "invalid-root", "url": hub_root}] + ) + cookies = await browser.context.cookies() + assert len(cookies) == 2 + + # after visiting page, cookies get re-established + await browser.goto(login_url) + cookies = await browser.context.cookies() + print(cookies) + cookie = cookies[0] + assert cookie['name'] == '_xsrf' + assert cookie["path"] == app.hub.base_url + + # next page visit, cookies don't change + await browser.goto(login_url) + cookies_2 = await browser.context.cookies() + assert cookies == cookies_2 + # login is successful + await login(browser, username, username) + + +def _cookie_dict(cookie_list): + """Convert list of cookies to dict of the form + + { 'path': {'key': {cookie} } } + """ + cookie_dict = {} + for cookie in cookie_list: + path_cookies = cookie_dict.setdefault(cookie['path'], {}) + path_cookies[cookie['name']] = cookie + return cookie_dict + + +async def test_singleuser_xsrf(app, browser, user, create_user_with_scopes, full_spawn): + # full login process, checking XSRF handling + # start two servers + target_user = user + target_start = asyncio.ensure_future(target_user.spawn()) + + browser_user = create_user_with_scopes("self", "access:servers") + # login browser_user + login_url = url_path_join(public_host(app), app.hub.base_url, "login") + await browser.goto(login_url) + await login(browser, browser_user.name, browser_user.name) + # end up at single-user + await expect(browser).to_have_url(re.compile(rf".*/user/{browser_user.name}/.*")) + # wait for target user to start, too + await target_start + await app.proxy.add_user(target_user) + + # visit target user, sets credentials for second server + await browser.goto(public_url(app, target_user)) + await expect(browser).to_have_url(re.compile(r".*/oauth2/authorize")) + auth_button = browser.locator('//input[@type="submit"]') + await expect(auth_button).to_be_enabled() + await auth_button.click() + await expect(browser).to_have_url(re.compile(rf".*/user/{target_user.name}/.*")) + + # at this point, we are on a page served by target_user, + # logged in as browser_user + # basic check that xsrf isolation works + cookies = await browser.context.cookies() + cookie_dict = _cookie_dict(cookies) + pprint.pprint(cookie_dict) + + # we should have xsrf tokens for both singleuser servers and the hub + target_prefix = target_user.prefix + user_prefix = browser_user.prefix + hub_prefix = app.hub.base_url + assert target_prefix in cookie_dict + assert user_prefix in cookie_dict + assert hub_prefix in cookie_dict + target_xsrf = cookie_dict[target_prefix].get("_xsrf", {}).get("value") + assert target_xsrf + user_xsrf = cookie_dict[user_prefix].get("_xsrf", {}).get("value") + assert user_xsrf + hub_xsrf = cookie_dict[hub_prefix].get("_xsrf", {}).get("value") + assert hub_xsrf + assert hub_xsrf != target_xsrf + assert hub_xsrf != user_xsrf + assert target_xsrf != user_xsrf + + # we are on a page served by target_user + # check that we can't access + + async def fetch_user_page(path, params=None): + url = url_path_join(public_url(app, browser_user), path) + if params: + url = url_concat(url, params) + status = await browser.evaluate( + """ + async (user_url) => { + try { + response = await fetch(user_url); + } catch (e) { + return 'error'; + } + return response.status; + } + """, + url, + ) + return status + + if app.subdomain_host: + expected_status = 'error' + else: + expected_status = 403 + status = await fetch_user_page("/api/contents") + assert status == expected_status + status = await fetch_user_page("/api/contents", params={"_xsrf": target_xsrf}) + assert status == expected_status + + if not app.subdomain_host: + expected_status = 200 + status = await fetch_user_page("/api/contents", params={"_xsrf": user_xsrf}) + assert status == expected_status + + # check that we can't iframe the other user's page + async def iframe(src): + return await browser.evaluate( + """ + async (src) => { + const frame = document.createElement("iframe"); + frame.src = src; + return new Promise((resolve, reject) => { + frame.addEventListener("load", (event) => { + if (frame.contentDocument) { + resolve("got document!"); + } else { + resolve("blocked") + } + }); + setTimeout(() => { + // some browsers (firefox) never fire load event + // despite spec appasrently stating it must always do so, + // even for rejected frames + resolve("timeout") + }, 3000) + + document.body.appendChild(frame); + }); + } + """, + src, + ) + + hub_iframe = await iframe(url_path_join(public_url(app), "hub/admin")) + assert hub_iframe in {"timeout", "blocked"} + user_iframe = await iframe(public_url(app, browser_user)) + assert user_iframe in {"timeout", "blocked"} + + # check that server page can still connect to its own kernels + token = target_user.new_api_token(scopes=["access:servers!user"]) + url = url_path_join(public_url(app, target_user), "/api/kernels") + headers = {"Authorization": f"Bearer {token}"} + r = await async_requests.post(url, headers=headers) + r.raise_for_status() + kernel = r.json() + kernel_id = kernel["id"] + kernel_url = url_path_join(url, kernel_id) + kernel_ws_url = "ws" + url_path_join(kernel_url, "channels")[4:] + try: + result = await browser.evaluate( + """ + async (ws_url) => { + ws = new WebSocket(ws_url); + finished = await new Promise((resolve, reject) => { + ws.onerror = (err) => { + reject(err); + }; + ws.onopen = () => { + resolve("ok"); + }; + }); + return finished; + } + """, + kernel_ws_url, + ) + finally: + r = await async_requests.delete(kernel_url, headers=headers) + r.raise_for_status() + assert result == "ok" diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index c4686829..041beb65 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -44,8 +44,8 @@ from .. import metrics, orm, roles from ..app import JupyterHub from ..auth import PAMAuthenticator from ..spawner import SimpleLocalProcessSpawner -from ..utils import random_port, utcnow -from .utils import async_requests, public_url, ssl_setup +from ..utils import random_port, url_path_join, utcnow +from .utils import AsyncSession, public_url, ssl_setup def mock_authenticate(username, password, service, encoding): @@ -372,29 +372,32 @@ class MockHub(JupyterHub): async def login_user(self, name): """Login a user by name, returning her cookies.""" base_url = public_url(self) - external_ca = None + s = AsyncSession() if self.internal_ssl: - external_ca = self.external_certs['files']['ca'] + s.verify = self.external_certs['files']['ca'] login_url = base_url + 'hub/login' - r = await async_requests.get(login_url) + r = await s.get(login_url) r.raise_for_status() xsrf = r.cookies['_xsrf'] - r = await async_requests.post( + r = await s.post( url_concat(login_url, {"_xsrf": xsrf}), - cookies=r.cookies, data={'username': name, 'password': name}, allow_redirects=False, - verify=external_ca, ) r.raise_for_status() - r.cookies["_xsrf"] = xsrf - assert sorted(r.cookies.keys()) == [ + # make second request to get updated xsrf cookie + r2 = await s.get( + url_path_join(base_url, "hub/home"), + allow_redirects=False, + ) + assert r2.status_code == 200 + assert sorted(s.cookies.keys()) == [ '_xsrf', 'jupyterhub-hub-login', 'jupyterhub-session-id', ] - return r.cookies + return s.cookies class InstrumentedSpawner(MockSpawner): diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 8639a038..ac81970c 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -99,7 +99,7 @@ async def test_post_content_type(app, content_type, status): assert r.status_code == status -@mark.parametrize("xsrf_in_url", [True, False]) +@mark.parametrize("xsrf_in_url", [True, False, "invalid"]) @mark.parametrize( "method, path", [ @@ -110,6 +110,13 @@ async def test_post_content_type(app, content_type, status): async def test_xsrf_check(app, username, method, path, xsrf_in_url): cookies = await app.login_user(username) xsrf = cookies['_xsrf'] + if xsrf_in_url == "invalid": + cookies.pop("_xsrf") + # a valid old-style tornado xsrf token is no longer valid + xsrf = cookies['_xsrf'] = ( + "2|7329b149|d837ced983e8aac7468bc7a61ce3d51a|1708610065" + ) + url = path.format(username=username) if xsrf_in_url: url = f"{url}?_xsrf={xsrf}" @@ -120,7 +127,7 @@ async def test_xsrf_check(app, username, method, path, xsrf_in_url): noauth=True, cookies=cookies, ) - if xsrf_in_url: + if xsrf_in_url is True: assert r.status_code == 200 else: assert r.status_code == 403 diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 52a42d4d..c07441ca 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -685,11 +685,10 @@ async def test_other_user_url(app, username, user, group, create_temp_role, has_ ], ) async def test_page_with_token(app, user, url, token_in): - cookies = await app.login_user(user.name) token = user.new_api_token() if token_in == "url": url = url_concat(url, {"token": token}) - headers = None + headers = {} elif token_in == "header": headers = { "Authorization": f"token {token}", @@ -734,14 +733,13 @@ async def test_login_strip(app, form_user, auth_user, form_password): """Test that login form strips space form usernames, but not passwords""" form_data = {"username": form_user, "password": form_password} expected_auth = {"username": auth_user, "password": form_password} - base_url = public_url(app) called_with = [] async def mock_authenticate(handler, data): called_with.append(data) with mock.patch.object(app.authenticator, 'authenticate', mock_authenticate): - r = await async_requests.get(base_url + 'hub/login') + r = await get_page('login', app) r.raise_for_status() cookies = r.cookies xsrf = cookies['_xsrf'] @@ -922,17 +920,19 @@ async def test_auto_login(app, request): async def test_auto_login_logout(app): name = 'burnham' cookies = await app.login_user(name) + s = AsyncSession() + s.cookies = cookies with mock.patch.dict( app.tornado_settings, {'authenticator': Authenticator(auto_login=True)} ): - r = await async_requests.get( + r = await s.get( public_host(app) + app.tornado_settings['logout_url'], cookies=cookies ) r.raise_for_status() logout_url = public_host(app) + app.tornado_settings['logout_url'] assert r.url == logout_url - assert r.cookies == {} + assert list(s.cookies.keys()) == ["_xsrf"] # don't include logged-out user in page: try: idx = r.text.index(name) @@ -946,19 +946,23 @@ async def test_auto_login_logout(app): async def test_logout(app): name = 'wash' cookies = await app.login_user(name) - r = await async_requests.get( - public_host(app) + app.tornado_settings['logout_url'], cookies=cookies + s = AsyncSession() + s.cookies = cookies + r = await s.get( + public_host(app) + app.tornado_settings['logout_url'], ) r.raise_for_status() login_url = public_host(app) + app.tornado_settings['login_url'] assert r.url == login_url - assert r.cookies == {} + assert list(s.cookies.keys()) == ["_xsrf"] @pytest.mark.parametrize('shutdown_on_logout', [True, False]) async def test_shutdown_on_logout(app, shutdown_on_logout): name = 'shutitdown' cookies = await app.login_user(name) + s = AsyncSession() + s.cookies = cookies user = app.users[name] # start the user's server @@ -978,14 +982,14 @@ async def test_shutdown_on_logout(app, shutdown_on_logout): with mock.patch.dict( app.tornado_settings, {'shutdown_on_logout': shutdown_on_logout} ): - r = await async_requests.get( + r = await s.get( public_host(app) + app.tornado_settings['logout_url'], cookies=cookies ) r.raise_for_status() login_url = public_host(app) + app.tornado_settings['login_url'] assert r.url == login_url - assert r.cookies == {} + assert list(s.cookies.keys()) == ["_xsrf"] # wait for any pending state to resolve for i in range(50): diff --git a/jupyterhub/tests/test_services_auth.py b/jupyterhub/tests/test_services_auth.py index 42c4fa72..ca836971 100644 --- a/jupyterhub/tests/test_services_auth.py +++ b/jupyterhub/tests/test_services_auth.py @@ -386,7 +386,7 @@ async def test_oauth_service_roles( # token-authenticated request to HubOAuth token = app.users[name].new_api_token() # token in ?token parameter - r = await async_requests.get(url_concat(url, {'token': token})) + r = await async_requests.get(url_concat(url, {'token': token}), headers=s.headers) r.raise_for_status() reply = r.json() assert reply['name'] == name @@ -394,7 +394,9 @@ async def test_oauth_service_roles( # verify that ?token= requests set a cookie assert len(r.cookies) != 0 # ensure cookie works in future requests - r = await async_requests.get(url, cookies=r.cookies, allow_redirects=False) + r = await async_requests.get( + url, cookies=r.cookies, allow_redirects=False, headers=s.headers + ) r.raise_for_status() assert r.url == url reply = r.json() diff --git a/jupyterhub/tests/test_singleuser.py b/jupyterhub/tests/test_singleuser.py index 9bb62183..938fcbb6 100644 --- a/jupyterhub/tests/test_singleuser.py +++ b/jupyterhub/tests/test_singleuser.py @@ -75,18 +75,20 @@ async def test_singleuser_auth( spawner = user.spawners[server_name] url = url_path_join(public_url(app, user), server_name) + s = AsyncSession() + # no cookies, redirects to login page - r = await async_requests.get(url) + r = await s.get(url) r.raise_for_status() assert '/hub/login' in r.url # unauthenticated /api/ should 403, not redirect api_url = url_path_join(url, "api/status") - r = await async_requests.get(api_url, allow_redirects=False) + r = await s.get(api_url, allow_redirects=False) assert r.status_code == 403 # with cookies, login successful - r = await async_requests.get(url, cookies=cookies) + r = await s.get(url, cookies=cookies) r.raise_for_status() assert ( urlparse(r.url) @@ -100,7 +102,7 @@ async def test_singleuser_auth( assert r.status_code == 200 # logout - r = await async_requests.get(url_path_join(url, 'logout'), cookies=cookies) + r = await s.get(url_path_join(url, 'logout')) assert len(r.cookies) == 0 # accessing another user's server hits the oauth confirmation page @@ -149,6 +151,8 @@ async def test_singleuser_auth( async def test_disable_user_config(request, app, tmp_path, full_spawn): # login, start the server cookies = await app.login_user('nandy') + s = AsyncSession() + s.cookies = cookies user = app.users['nandy'] # stop spawner, if running: if user.running: @@ -180,10 +184,11 @@ async def test_disable_user_config(request, app, tmp_path, full_spawn): url = public_url(app, user) # with cookies, login successful - r = await async_requests.get(url, cookies=cookies) + r = await s.get(url) r.raise_for_status() assert r.url.endswith('/user/nandy/jupyterhub-test-info') assert r.status_code == 200 + info = r.json() pprint(info) assert info['disable_user_config'] @@ -385,20 +390,31 @@ async def test_nbclassic_control_panel(app, user, full_spawn): @pytest.mark.skipif( IS_JUPYVERSE, reason="jupyverse doesn't implement token authentication" ) -async def test_token_url_cookie(app, user, full_spawn): +@pytest.mark.parametrize("accept_token_in_url", ["1", "0", ""]) +async def test_token_url_cookie(app, user, full_spawn, accept_token_in_url): + if accept_token_in_url: + user.spawner.environment["JUPYTERHUB_ALLOW_TOKEN_IN_URL"] = accept_token_in_url + should_accept = accept_token_in_url != "0" + await user.spawn() await app.proxy.add_user(user) + token = user.new_api_token(scopes=["access:servers!user"]) url = url_path_join(public_url(app, user), user.spawner.default_url or "/tree/") # first request: auth with token in URL - r = await async_requests.get(url + f"?token={token}", allow_redirects=False) + s = AsyncSession() + r = await s.get(url + f"?token={token}", allow_redirects=False) print(r.url, r.status_code) + if not should_accept: + assert r.status_code == 302 + return + assert r.status_code == 200 - assert r.cookies + assert s.cookies # second request, use cookies set by first response, # no token in URL - r = await async_requests.get(url, cookies=r.cookies, allow_redirects=False) + r = await s.get(url, allow_redirects=False) assert r.status_code == 200 await user.stop() @@ -409,7 +425,8 @@ async def test_api_403_no_cookie(app, user, full_spawn): await user.spawn() await app.proxy.add_user(user) url = url_path_join(public_url(app, user), "/api/contents/") - r = await async_requests.get(url, allow_redirects=False) + s = AsyncSession() + r = await s.get(url, allow_redirects=False) # 403, not redirect assert r.status_code == 403 # no state cookie set diff --git a/jupyterhub/tests/utils.py b/jupyterhub/tests/utils.py index 54222658..143b2a9a 100644 --- a/jupyterhub/tests/utils.py +++ b/jupyterhub/tests/utils.py @@ -42,6 +42,13 @@ async_requests = _AsyncRequests() class AsyncSession(requests.Session): """requests.Session object that runs in the background thread""" + def __init__(self, **kwargs): + super().__init__(**kwargs) + # session requests are for cookie authentication + # and should look like regular page views, + # so set Sec-Fetch-Mode: navigate + self.headers.setdefault("Sec-Fetch-Mode", "navigate") + def request(self, *args, **kwargs): return async_requests.executor.submit(super().request, *args, **kwargs) @@ -157,6 +164,7 @@ async def api_request( else: base_url = public_url(app, path='hub') headers = kwargs.setdefault('headers', {}) + headers.setdefault("Sec-Fetch-Mode", "cors") if 'Authorization' not in headers and not noauth and 'cookies' not in kwargs: # make a copy to avoid modifying arg in-place kwargs['headers'] = h = {} @@ -176,7 +184,7 @@ async def api_request( kwargs['cert'] = (app.internal_ssl_cert, app.internal_ssl_key) kwargs["verify"] = app.internal_ssl_ca resp = await f(url, **kwargs) - assert "frame-ancestors 'self'" in resp.headers['Content-Security-Policy'] + assert "frame-ancestors 'none'" in resp.headers['Content-Security-Policy'] assert ( ujoin(app.hub.base_url, "security/csp-report") in resp.headers['Content-Security-Policy'] @@ -197,6 +205,9 @@ def get_page(path, app, hub=True, **kw): else: prefix = app.base_url base_url = ujoin(public_host(app), prefix) + # Sec-Fetch-Mode=navigate to look like a regular page view + headers = kw.setdefault("headers", {}) + headers.setdefault("Sec-Fetch-Mode", "navigate") return async_requests.get(ujoin(base_url, path), **kw) diff --git a/jupyterhub/user.py b/jupyterhub/user.py index 299e5b91..3e96ee9f 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -426,6 +426,9 @@ class User: _deprecated_db_session=self.db, oauth_client_id=client_id, cookie_options=self.settings.get('cookie_options', {}), + cookie_host_prefix_enabled=self.settings.get( + "cookie_host_prefix_enabled", False + ), trusted_alt_names=trusted_alt_names, user_options=orm_spawner.user_options or {}, ) diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index 129c5944..6a9ee8c2 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -8,6 +8,7 @@ import errno import functools import hashlib import inspect +import os import random import re import secrets @@ -34,6 +35,21 @@ from tornado.httpclient import AsyncHTTPClient, HTTPError from tornado.log import app_log +def _bool_env(key, default=False): + """Cast an environment variable to bool + + If unset or empty, return `default` + `0` is False; all other values are True. + """ + value = os.environ.get(key, "") + if value == "": + return default + if value.lower() in {"0", "false"}: + return False + else: + return True + + # Deprecated aliases: no longer needed now that we require 3.7 def asyncio_all_tasks(loop=None): warnings.warn( From b319b58a2f87e87f4f3af2ea2042e045c19ff866 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 19 Mar 2024 18:46:51 +0100 Subject: [PATCH 06/43] default=False for allow_token_in_url for 5.0 --- jupyterhub/services/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index d974dcdc..2f6566bb 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -360,7 +360,7 @@ class HubAuth(SingletonConfigurable): ).tag(config=True) allow_token_in_url = Bool( - _bool_env("JUPYTERHUB_ALLOW_TOKEN_IN_URL", default=True), + _bool_env("JUPYTERHUB_ALLOW_TOKEN_IN_URL", default=False), help="""Allow requests to pages with ?token=... in the URL This allows starting a user session by sharing a URL with credentials, From 66c1600f4f3fa4ad3c86d1a5b361c15f827ad3f9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 20 Mar 2024 12:22:38 +0100 Subject: [PATCH 07/43] run browser tests in subdomain --- .github/workflows/test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dd551181..98b0fe4b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -103,6 +103,9 @@ jobs: subset: singleuser - python: "3.11" browser: browser + - python: "3.11" + subdomain: subdomain + browser: browser - python: "3.12" main_dependencies: main_dependencies From 71f6cfa92b31e7cd9c23d81b4c3511e81788b047 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 20 Mar 2024 12:21:22 +0100 Subject: [PATCH 08/43] fix permission check on /hub/user/ page needed for share redirect to work --- jupyterhub/handlers/base.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 9c426ce3..65bf4e90 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -38,7 +38,6 @@ from ..metrics import ( ServerStopStatus, ) from ..objects import Server -from ..scopes import needs_scope from ..spawner import LocalProcessSpawner from ..user import User from ..utils import ( @@ -1557,10 +1556,28 @@ class UserUrlHandler(BaseHandler): delete = non_get @web.authenticated - @needs_scope("access:servers") async def get(self, user_name, user_path): if not user_path: user_path = '/' + path_parts = user_path.split("/", 2) + server_names = [""] + if len(path_parts) >= 3: + # second part _may_ be a server name + server_names.append(path_parts[1]) + + access_scopes = [ + f"access:servers!server={user_name}/{server_name}" + for server_name in server_names + ] + if not any(self.has_scope(scope) for scope in access_scopes): + self.log.warning( + "Not authorizing access to %s. Requires any of [%s], not derived from scopes [%s]", + self.request.path, + ", ".join(access_scopes), + ", ".join(self.expanded_scopes), + ) + raise web.HTTPError(404, "No access to resources or resources not found") + current_user = self.current_user if user_name != current_user.name: user = self.find_user(user_name) From 51156a4762139e0c296ad84c95c65a8edaf49a8f Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 19 Mar 2024 14:17:05 +0100 Subject: [PATCH 09/43] avoid duplicate jupyterhub installation almost every time installing docs/requirements.txt happens, JupyterHub is already installed adding an `--editable` here ensures a full rebuild happens every time, which is very slow --- .github/workflows/test-docs.yml | 2 +- .readthedocs.yaml | 1 + docs/requirements.txt | 13 +++---------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/.github/workflows/test-docs.yml b/.github/workflows/test-docs.yml index eb83d33b..97bdfa60 100644 --- a/.github/workflows/test-docs.yml +++ b/.github/workflows/test-docs.yml @@ -64,7 +64,7 @@ jobs: - name: Install requirements run: | - pip install -r docs/requirements.txt pytest + pip install -e . -r docs/requirements.txt pytest - name: pytest docs/ run: | diff --git a/.readthedocs.yaml b/.readthedocs.yaml index 1b783e0a..432093fd 100644 --- a/.readthedocs.yaml +++ b/.readthedocs.yaml @@ -15,6 +15,7 @@ build: python: install: + - path: . - requirements: docs/requirements.txt formats: diff --git a/docs/requirements.txt b/docs/requirements.txt index 54683842..5f174d5a 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -1,13 +1,6 @@ -# We install the jupyterhub package to help autodoc-traits inspect it and -# generate documentation. -# -# FIXME: If there is a way for this requirements.txt file to pass a flag that -# the build system can intercept to not build the javascript artifacts, -# then do so so. That would mean that installing the documentation can -# avoid needing node/npm installed. -# ---editable . - +# docs also require jupyterhub itself to be installed +# don't depend on it here, as that often results in a duplicate +# installation of jupyterhub that's already installed autodoc-traits jupyterhub-sphinx-theme myst-parser>=0.19 From 1cd3bc1860266dc80f45b7509eb4a001de34a9b1 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 20 Mar 2024 12:44:25 +0100 Subject: [PATCH 10/43] fix browser tests with subdomains --- jupyterhub/tests/browser/test_browser.py | 27 ++++++++++++++---------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/jupyterhub/tests/browser/test_browser.py b/jupyterhub/tests/browser/test_browser.py index 9054138a..5e698c66 100644 --- a/jupyterhub/tests/browser/test_browser.py +++ b/jupyterhub/tests/browser/test_browser.py @@ -44,7 +44,7 @@ async def test_submit_login_form(app, browser, user_special_chars): login_url = url_path_join(public_host(app), app.hub.base_url, "login") await browser.goto(login_url) await login(browser, user.name, password=user.name) - expected_url = ujoin(public_url(app), f"/user/{user_special_chars.urlname}/") + expected_url = public_url(app, user) await expect(browser).to_have_url(expected_url) @@ -56,7 +56,7 @@ async def test_submit_login_form(app, browser, user_special_chars): # will encode given parameters for an unauthenticated URL in the next url # the next parameter will contain the app base URL (replaces BASE_URL in tests) 'spawn', - [('param', 'value')], + {'param': 'value'}, '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', ), @@ -64,15 +64,15 @@ async def test_submit_login_form(app, browser, user_special_chars): # login?param=fromlogin&next=encoded(/hub/spawn?param=value) # will drop parameters given to the login page, passing only the next url 'login', - [('param', 'fromlogin'), ('next', '/hub/spawn?param=value')], - '/hub/login?param=fromlogin&next=%2Fhub%2Fspawn%3Fparam%3Dvalue', - '/hub/login?next=%2Fhub%2Fspawn%3Fparam%3Dvalue', + {'param': 'fromlogin', 'next': '/hub/spawn?param=value'}, + '/hub/login?param=fromlogin&next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', + '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue', ), ( # login?param=value&anotherparam=anothervalue # will drop parameters given to the login page, and use an empty next url 'login', - [('param', 'value'), ('anotherparam', 'anothervalue')], + {'param': 'value', 'anotherparam': 'anothervalue'}, '/hub/login?param=value&anotherparam=anothervalue', '/hub/login?next=', ), @@ -80,7 +80,7 @@ async def test_submit_login_form(app, browser, user_special_chars): # login # simplest case, accessing the login URL, gives an empty next url 'login', - [], + {}, '/hub/login', '/hub/login?next=', ), @@ -98,6 +98,8 @@ async def test_open_url_login( user = user_special_chars.user login_url = url_path_join(public_host(app), app.hub.base_url, url) await browser.goto(login_url) + if params.get("next"): + params["next"] = url_path_join(app.base_url, params["next"]) url_new = url_path_join(public_host(app), app.hub.base_url, url_concat(url, params)) print(url_new) await browser.goto(url_new) @@ -853,12 +855,15 @@ async def test_oauth_page( oauth_client.allowed_scopes = sorted(roles.roles_to_scopes([service_role])) app.db.commit() # open the service url in the browser - service_url = url_path_join(public_url(app, service) + 'owhoami/?arg=x') + service_url = url_path_join(public_url(app, service), 'owhoami/?arg=x') await browser.goto(service_url) - expected_redirect_url = url_path_join( - app.base_url + f"services/{service.name}/oauth_callback" - ) + if app.subdomain_host: + expected_redirect_url = url_path_join( + public_url(app, service), "oauth_callback" + ) + else: + expected_redirect_url = url_path_join(service.prefix, "oauth_callback") expected_client_id = f"service-{service.name}" # decode the URL From d2a07aaf1be10e156395d09d2e5eb8ca8f080f20 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 20 Mar 2024 13:21:34 +0100 Subject: [PATCH 11/43] forward-port 4.1.0 --- docs/source/conf.py | 2 ++ jupyterhub/services/auth.py | 1 + jupyterhub/tests/test_services_auth.py | 44 +++++--------------------- jupyterhub/tests/test_singleuser.py | 2 +- 4 files changed, 12 insertions(+), 37 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index 7f80b823..213b32e6 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -289,6 +289,8 @@ linkcheck_ignore = [ "https://github.com/jupyterhub/jupyterhub/compare/", # too many comparisons in changelog r"https?://(localhost|127.0.0.1).*", # ignore localhost references in auto-links r"https://linux.die.net/.*", # linux.die.net seems to block requests from CI with 403 sometimes + # don't check links to unpublished advisories + r"https://github.com/jupyterhub/jupyterhub/security/advisories/.*", ] linkcheck_anchors_ignore = [ "/#!", diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index 2f6566bb..b270f3b2 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -1203,6 +1203,7 @@ class HubOAuth(HubAuth): for cookie_name, cookie in handler.request.cookies.items(): if cookie_name.startswith(self.state_cookie_name): self._clear_cookie( + handler, cookie_name, path=self.cookie_path, ) diff --git a/jupyterhub/tests/test_services_auth.py b/jupyterhub/tests/test_services_auth.py index ca836971..e5aa3349 100644 --- a/jupyterhub/tests/test_services_auth.py +++ b/jupyterhub/tests/test_services_auth.py @@ -86,17 +86,9 @@ async def test_hubauth_token(app, mockservice_url, create_user_with_scopes): sub_reply = {key: reply.get(key, 'missing') for key in ['name', 'admin']} assert sub_reply == {'name': u.name, 'admin': False} - # token in ?token parameter + # token in ?token parameter is not allowed by default r = await async_requests.get( - public_url(app, mockservice_url) + '/whoami/?token=%s' % token - ) - r.raise_for_status() - reply = r.json() - sub_reply = {key: reply.get(key, 'missing') for key in ['name', 'admin']} - assert sub_reply == {'name': u.name, 'admin': False} - - r = await async_requests.get( - public_url(app, mockservice_url) + '/whoami/?token=no-such-token', + public_url(app, mockservice_url) + '/whoami/?token=%s' % token, allow_redirects=False, ) assert r.status_code == 302 @@ -180,21 +172,9 @@ async def test_hubauth_service_token(request, app, mockservice_url, scopes, allo else: assert r.status_code == 403 - # token in ?token parameter + # token in ?token parameter is not allowed by default r = await async_requests.get( - public_url(app, mockservice_url) + 'whoami/?token=%s' % token - ) - if allowed: - r.raise_for_status() - assert r.status_code == 200 - reply = r.json() - assert service_model.items() <= reply.items() - assert not r.cookies - else: - assert r.status_code == 403 - - r = await async_requests.get( - public_url(app, mockservice_url) + 'whoami/?token=no-such-token', + public_url(app, mockservice_url) + 'whoami/?token=%s' % token, allow_redirects=False, ) assert r.status_code == 302 @@ -385,22 +365,14 @@ async def test_oauth_service_roles( # token-authenticated request to HubOAuth token = app.users[name].new_api_token() - # token in ?token parameter - r = await async_requests.get(url_concat(url, {'token': token}), headers=s.headers) + s.headers["Authorization"] = f"Bearer {token}" + r = await async_requests.get(url, headers=s.headers) r.raise_for_status() reply = r.json() assert reply['name'] == name - # verify that ?token= requests set a cookie - assert len(r.cookies) != 0 - # ensure cookie works in future requests - r = await async_requests.get( - url, cookies=r.cookies, allow_redirects=False, headers=s.headers - ) - r.raise_for_status() - assert r.url == url - reply = r.json() - assert reply['name'] == name + # tokens in headers don't set cookies + assert len(r.cookies) == 0 @pytest.mark.parametrize( diff --git a/jupyterhub/tests/test_singleuser.py b/jupyterhub/tests/test_singleuser.py index 938fcbb6..f5b16763 100644 --- a/jupyterhub/tests/test_singleuser.py +++ b/jupyterhub/tests/test_singleuser.py @@ -394,7 +394,7 @@ async def test_nbclassic_control_panel(app, user, full_spawn): async def test_token_url_cookie(app, user, full_spawn, accept_token_in_url): if accept_token_in_url: user.spawner.environment["JUPYTERHUB_ALLOW_TOKEN_IN_URL"] = accept_token_in_url - should_accept = accept_token_in_url != "0" + should_accept = accept_token_in_url == "1" await user.spawn() await app.proxy.add_user(user) From ff693e82af297446312b6da32f1e33af7afb42c4 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 20 Mar 2024 14:37:54 +0100 Subject: [PATCH 12/43] set login cookie if user changed not just if unset allows login _override_ of existing user without needing to log out first --- jupyterhub/handlers/base.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index c4ec2bac..8f866b12 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -739,8 +739,11 @@ class BaseHandler(RequestHandler): if not self.get_session_cookie(): self.set_session_cookie() - # create and set a new cookie token for the hub - if not self.get_current_user_cookie(): + # create and set a new cookie for the hub + cookie_user = self.get_current_user_cookie() + if cookie_user is None or cookie_user.id != user.id: + if cookie_user: + self.log.info(f"User {cookie_user.name} is logging in as {user.name}") self.set_hub_cookie(user) def authenticate(self, data): From 92d59cd12b3697ad5977720cd975e25f722cb6b2 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Thu, 21 Mar 2024 20:00:42 +0000 Subject: [PATCH 13/43] docs: Consistently use minimum Python 3.8 --- README.md | 2 +- docs/source/conf.py | 1 + docs/source/contributing/setup.md | 4 ++-- docs/source/tutorial/quickstart.md | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index e500f931..290ab2c9 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ for administration of the Hub and its users. ### Check prerequisites - A Linux/Unix based system -- [Python](https://www.python.org/downloads/) 3.6 or greater +- [Python](https://www.python.org/downloads/) 3.8 or greater - [nodejs/npm](https://www.npmjs.com/) - If you are using **`conda`**, the nodejs and npm dependencies will be installed for diff --git a/docs/source/conf.py b/docs/source/conf.py index 213b32e6..9e952e8e 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -70,6 +70,7 @@ myst_enable_extensions = [ myst_substitutions = { # date example: Dev 07, 2022 "date": datetime.date.today().strftime("%b %d, %Y").title(), + "python_min": "3.8", "version": jupyterhub.__version__, } diff --git a/docs/source/contributing/setup.md b/docs/source/contributing/setup.md index eaea5957..1edf0ca5 100644 --- a/docs/source/contributing/setup.md +++ b/docs/source/contributing/setup.md @@ -12,7 +12,7 @@ development. ### Install Python JupyterHub is written in the [Python](https://python.org) programming language and -requires you have at least version 3.8 installed locally. If you haven’t +requires you have at least version {{python_min}} installed locally. If you haven’t installed Python before, the recommended way to install it is to use [Miniforge](https://github.com/conda-forge/miniforge#download). @@ -59,7 +59,7 @@ a more detailed discussion. python -V ``` - This should return a version number greater than or equal to 3.8. + This should return a version number greater than or equal to {{python_min}}. ```bash npm -v diff --git a/docs/source/tutorial/quickstart.md b/docs/source/tutorial/quickstart.md index 13a8e916..b3700586 100644 --- a/docs/source/tutorial/quickstart.md +++ b/docs/source/tutorial/quickstart.md @@ -5,7 +5,7 @@ Before installing JupyterHub, you will need: - a Linux/Unix-based system -- [Python](https://www.python.org/downloads/) 3.6 or greater. An understanding +- [Python {{python_min}}](https://www.python.org/downloads/) or greater. An understanding of using [`pip`](https://pip.pypa.io) or [`conda`](https://docs.conda.io/projects/conda/en/latest/user-guide/getting-started.html) for installing Python packages is helpful. From acf7d7daaa0dc125dcd23a936638a401b8162a97 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Thu, 21 Mar 2024 20:01:40 +0000 Subject: [PATCH 14/43] docs: use sphinx var for min node version --- docs/source/conf.py | 1 + docs/source/contributing/setup.md | 2 +- docs/source/tutorial/quickstart.md | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index 9e952e8e..4e357d87 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -70,6 +70,7 @@ myst_enable_extensions = [ myst_substitutions = { # date example: Dev 07, 2022 "date": datetime.date.today().strftime("%b %d, %Y").title(), + "node_min": "12", "python_min": "3.8", "version": jupyterhub.__version__, } diff --git a/docs/source/contributing/setup.md b/docs/source/contributing/setup.md index 1edf0ca5..9b6cf9f3 100644 --- a/docs/source/contributing/setup.md +++ b/docs/source/contributing/setup.md @@ -18,7 +18,7 @@ installed Python before, the recommended way to install it is to use ### Install nodejs -[NodeJS 12+](https://nodejs.org/en/) is required for building some JavaScript components. +[NodeJS {{node_min}}+](https://nodejs.org/en/) is required for building some JavaScript components. `configurable-http-proxy`, the default proxy implementation for JupyterHub, is written in Javascript. If you have not installed NodeJS before, we recommend installing it in the `miniconda` environment you set up for Python. You can do so with `conda install nodejs`. diff --git a/docs/source/tutorial/quickstart.md b/docs/source/tutorial/quickstart.md index b3700586..eca85a9e 100644 --- a/docs/source/tutorial/quickstart.md +++ b/docs/source/tutorial/quickstart.md @@ -9,7 +9,7 @@ Before installing JupyterHub, you will need: of using [`pip`](https://pip.pypa.io) or [`conda`](https://docs.conda.io/projects/conda/en/latest/user-guide/getting-started.html) for installing Python packages is helpful. -- [nodejs/npm](https://www.npmjs.com/). [Install nodejs/npm](https://docs.npmjs.com/getting-started/installing-node), +- [Node.js {{node_min}}](https://www.npmjs.com/) or greater, along with npm. [Install Node.js/npm](https://docs.npmjs.com/getting-started/installing-node), using your operating system's package manager. - If you are using **`conda`**, the nodejs and npm dependencies will be installed for @@ -24,7 +24,7 @@ Before installing JupyterHub, you will need: ``` [nodesource][] is a great resource to get more recent versions of the nodejs runtime, - if your system package manager only has an old version of Node.js (e.g. 10 or older). + if your system package manager only has an old version of Node.js. - A [pluggable authentication module (PAM)](https://en.wikipedia.org/wiki/Pluggable_authentication_module) to use the [default Authenticator](authenticators). From 352826a1ec4c75aaf8892d73082e5d0af0bcb9aa Mon Sep 17 00:00:00 2001 From: Simon Li Date: Thu, 21 Mar 2024 20:01:54 +0000 Subject: [PATCH 15/43] docs: fix unrelated rendering error --- docs/source/contributing/setup.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/contributing/setup.md b/docs/source/contributing/setup.md index 9b6cf9f3..92da6428 100644 --- a/docs/source/contributing/setup.md +++ b/docs/source/contributing/setup.md @@ -23,7 +23,7 @@ installed Python before, the recommended way to install it is to use If you have not installed NodeJS before, we recommend installing it in the `miniconda` environment you set up for Python. You can do so with `conda install nodejs`. -Many in the Jupyter community use \[`nvm`\]() to +Many in the Jupyter community use [`nvm`](https://github.com/nvm-sh/nvm) to managing node dependencies. ### Install git From 9b3d55ded0338ff3a5ebce7cde0d998e1617740d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 22 Mar 2024 08:07:23 +0000 Subject: [PATCH 16/43] Bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /jsx Bumps [webpack-dev-middleware](https://github.com/webpack/webpack-dev-middleware) from 5.3.3 to 5.3.4. - [Release notes](https://github.com/webpack/webpack-dev-middleware/releases) - [Changelog](https://github.com/webpack/webpack-dev-middleware/blob/v5.3.4/CHANGELOG.md) - [Commits](https://github.com/webpack/webpack-dev-middleware/compare/v5.3.3...v5.3.4) --- updated-dependencies: - dependency-name: webpack-dev-middleware dependency-type: indirect ... Signed-off-by: dependabot[bot] --- jsx/package-lock.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/jsx/package-lock.json b/jsx/package-lock.json index ef1e0f4b..b8485724 100644 --- a/jsx/package-lock.json +++ b/jsx/package-lock.json @@ -9797,9 +9797,10 @@ } }, "node_modules/webpack-dev-middleware": { - "version": "5.3.3", + "version": "5.3.4", + "resolved": "https://registry.npmjs.org/webpack-dev-middleware/-/webpack-dev-middleware-5.3.4.tgz", + "integrity": "sha512-BVdTqhhs+0IfoeAf7EoH5WE+exCmqGerHfDM0IL096Px60Tq2Mn9MAbnaGUe6HiMa41KMCYF19gyzZmBcq/o4Q==", "dev": true, - "license": "MIT", "dependencies": { "colorette": "^2.0.10", "memfs": "^3.4.3", From ca3ac3b08b56d601ad641604e0cb012b8e9a80e6 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 22 Mar 2024 09:20:12 +0100 Subject: [PATCH 17/43] Officially adopt EffVer encodes the policy we already have, but now it has a name --- docs/source/reference/changelog.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index 0ffc2b1e..5e507580 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -6,6 +6,18 @@ For detailed changes from the prior release, click on the version number, and its link will bring up a GitHub listing of changes. Use `git log` on the command line for details. +## Versioning + +JupyterHub follows Intended Effort Versioning ([EffVer](https://jacobtomlinson.dev/effver/)) for versioning, +where the version number is meant to indicate the amount of effort required to upgrade to the new version. + +Contributors to major version bumps in JupyterHub include: + +- Database schema changes that require migrations and are hard to roll back +- Increasing the minimum required Python version +- Large new features +- Breaking changes likely to affect users + ## [Unreleased] ## 4.1 From c3c69027fa6c8479c5e1f651dc04eeddc256cdb3 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 22 Mar 2024 15:46:03 +0100 Subject: [PATCH 18/43] set allow_all=False by default --- docs/source/reference/authenticators.md | 182 +++++++++++++----- .../authenticators-users-basics.md | 54 ++++-- jupyterhub/app.py | 13 +- jupyterhub/auth.py | 144 +++++++++----- jupyterhub/tests/mocking.py | 1 + jupyterhub/tests/test_auth.py | 79 +++++--- 6 files changed, 333 insertions(+), 140 deletions(-) diff --git a/docs/source/reference/authenticators.md b/docs/source/reference/authenticators.md index 1508a092..8c662c4d 100644 --- a/docs/source/reference/authenticators.md +++ b/docs/source/reference/authenticators.md @@ -41,6 +41,11 @@ When testing, it may be helpful to use the password unless a global password has been set. Once set, any username will still be accepted but the correct password will need to be provided. +:::{versionadded} 5.0 +The DummyAuthenticator's default `allow_all` is True, +unlike most other Authenticators. +::: + ## Additional Authenticators Additional authenticators can be found on GitHub @@ -81,6 +86,7 @@ Writing an Authenticator that looks up passwords in a dictionary requires only overriding this one method: ```python +from secrets import compare_digest from traitlets import Dict from jupyterhub.auth import Authenticator @@ -91,8 +97,14 @@ class DictionaryAuthenticator(Authenticator): ) async def authenticate(self, handler, data): - if self.passwords.get(data['username']) == data['password']: - return data['username'] + username = data["username"] + password = data["password"] + check_password = self.passwords.get(username, "") + # always call compare_digest, for timing attacks + if compare_digest(check_password, password) and username in self.passwords: + return username + else: + return None ``` #### Normalize usernames @@ -190,10 +202,10 @@ via `jupyterhub --generate-config`. When dealing with logging in, there are generally two _separate_ steps: authentication -: identifying who is logged in, and +: identifying who is trying to log in, and authorization -: deciding whether an authenticated user is logged in +: deciding whether an authenticated user is allowed to access your JupyterHub {meth}`Authenticator.authenticate` is responsible for authenticating users. It is perfectly fine in the simplest cases for `Authenticator.authenticate` to be responsible for authentication _and_ authorization, @@ -203,71 +215,137 @@ However, Authenticators also have have two methods {meth}`~.Authenticator.check_ If `check_blocked_users()` returns False, authorization stops and the user is not allowed. -If `check_allowed()` returns True, authorization proceeds. +If `Authenticator.allow_all` is True OR `check_allowed()` returns True, authorization proceeds. :::{versionadded} 5.0 -{attr}`Authenticator.allow_all` and {attr}`Authenticator.allow_existing_users` are new in JupyterHub 5.0. +{attr}`.Authenticator.allow_all` and {attr}`.Authenticator.allow_existing_users` are new in JupyterHub 5.0. -By default, `allow_all` is True when `allowed_users` is empty, -and `allow_existing_users` is True when `allowed_users` is not empty. -This is to ensure backward-compatibility, but subclasses are free to pick more restrictive defaults. +By default, `allow_all` is False, +which is a change from pre-5.0, where `allow_all` was implicitly True if `allowed_users` was empty. ::: ### Overriding `check_allowed` +:::{versionchanged} 5.0 +`check_allowed()` is **not called** is `allow_all` is True. +::: + +:::{versionchanged} 5.0 +Starting with 5.0, `check_allowed()` should **NOT** return True if no allow config +is specified (`allow_all` should be used instead). + +::: + The base implementation of {meth}`~.Authenticator.check_allowed` checks: -- if `allow_all` is True, return True - if username is in the `allowed_users` set, return True - else return False -If a custom Authenticator defines additional sources of `allow` configuration, -such as membership in a group or other information, -it should override `check_allowed` to account for this. -`allow_` configuration should generally be _additive_, -i.e. if permission is granted by _any_ allow configuration, -a user should be authorized. - -:::{note} -For backward-compatibility, it is the responsibility of `Authenticator.check_allowed()` to check `.allow_all`. -This is to avoid the backward-compatible default values from granting permissions unexpectedly. -::: - -If an Authenticator defines additional `allow` configuration, it must at least: - -1. override `check_allowed`, and -2. override the default for `allow_all` - -The default for `allow_all` in a custom authenticator should be one of `False` or a dynamic default matching something like `if not any allow configuration specified`. -False is recommended for authenticators which source much larger pools of users than are _typically_ allowed to access a Hub (e.g. generic OAuth providers like Google, GitHub, etc.). - -For example, here is how `PAMAuthenticator` extends the base class to add `allowed_groups`: +:::{versionchanged} 5.0 +Prior to 5.0, the check was ```python -from traitlets import default - -@default("allow_all") -def _allow_all_default(self): - if self.allowed_users or self.allowed_groups: - # if any allow config is specified, default to False - return False - return True - -def check_allowed(self, username, authentication=None): - if self.allow_all: - return True - if self.check_allowed_groups(username, authentication): - return True - return super().check_allowed(username, authentication) +if (not allowed_users) or username in allowed_users: ``` -Important points to note: +but the implicit `not allowed_users` has been replaced by explicit `allow_all`, which is checked _before_ calling `check_allowed`. +`check_allowed` **is not called** if `allow_all` is True. -- overriding the default for `allow_all` is required to avoid `allow_all` being True when `allowed_groups` is specified, but `allowed_users` is not. -- `allow_all` must be checked inside `check_allowed` -- `allowed_groups` strictly expands who is authorized, - it does not apply restrictions `allowed_users`. - This is recommended for all `allow_` configuration added by Authenticators. +If your Authenticator subclass similarly returns True when no allow config is defined, +this is fully backward compatible for your users, but means `allow_all = False` has no real effect. + +You can make your Authenticator forward-compatible with JupyterHub 5 by defining `allow_all` as a boolean config trait on your class: + +```python +class MyAuthenticator(Authenticator): + + # backport allow_all from JupyterHub 5 + allow_all = Bool(False, config=True) + + def check_allowed(self, username, authentication): + if self.allow_all: + # replaces previous "if no auth config" + return True + ... +``` + +::: + +If an Authenticator defines additional sources of `allow` configuration, +such as membership in a group or other information, +it should override `check_allowed` to account for this. + +:::{note} +`allow_` configuration should generally be _additive_, +i.e. if access is granted by _any_ allow configuration, +a user should be authorized. + +JupyterHub recommends that Authenticators applying _restrictive_ configuration should use names like `block_` or `require_`, +and check this during `check_blocked_users` or `authenticate`, not `check_allowed`. +::: + +In general, an Authenticator's skeleton should look like: + +```python +class MyAuthenticator(Authenticator): + # backport allow_all for compatibility with JupyterHub < 5 + allow_all = Bool(False, config=True) + require_something = List(config=True) + allowed_something = Set() + + def authenticate(self, data, handler): + ... + if success: + return {"username": username, "auth_state": {...}} + else: + return None + + def check_blocked_users(self, username, authentication=None): + """Apply _restrictive_ configuration""" + + if self.require_something and not has_something(username, self.request_): + return False + # repeat for each restriction + if restriction_defined and restriction_not_met: + return False + return super().check_blocked_users(self, username, authentication) + + def check_allowed(self, username, authentication=None): + """Apply _permissive_ configuration + + Only called if check_blocked_users returns True + AND allow_all is False + """ + if self.allow_all: + # check here to backport allow_all behavior + # from JupyterHub 5 + # this branch will never be taken with jupyterhub >=5 + return True + if self.allowed_something and user_has_something(username): + return True + # repeat for each allow + if allow_config and allow_met: + return True + # should always have this at the end + if self.allowed_users and username in self.allowed_users: + return True + # do not call super! + # super().check_allowed is not safe with JupyterHub < 5.0, + # as it will return True if allowed_users is empty + return False +``` + +Key points: + +- `allow_all` is backported from JupyterHub 5, for consistent behavior in all versions of JupyterHub (optional) +- restrictive configuration is checked in `check_blocked_users` +- if any restriction is not met, `check_blocked_users` returns False +- permissive configuration is checked in `check_allowed` +- if any `allow` condition is met, `check_allowed` returns True + +So the logical expression for a user being authorized should look like: + +> if ALL restrictions are met AND ANY admissions are met: user is authorized #### Custom error messages diff --git a/docs/source/tutorial/getting-started/authenticators-users-basics.md b/docs/source/tutorial/getting-started/authenticators-users-basics.md index 4b2b4e2f..6726091e 100644 --- a/docs/source/tutorial/getting-started/authenticators-users-basics.md +++ b/docs/source/tutorial/getting-started/authenticators-users-basics.md @@ -6,30 +6,57 @@ The default Authenticator uses [PAM][] (Pluggable Authentication Module) to auth their usernames and passwords. With the default Authenticator, any user with an account and password on the system will be allowed to login. -## Create a set of allowed users (`allowed_users`) +## Deciding who is allowed + +In the base Authenticator, there are 3 configuration options for granting users access to your Hub: + +1. `allow_all` grants any user who can successfully authenticate access to the Hub +2. `allowed_users` defines a set of users who can access the Hub +3. `allow_existing_users` enables managing users via the JupyterHub API or admin page + +These options should apply to all Authenticators. +Your chosen Authenticator may add additional configuration options to admit users, such as team membership, course enrollment, etc. + +:::{important} +You should always specify at least one allow configuration if you want people to be able to access your Hub! +In most cases, this looks like: + +```python +c.Authenticator.allow_all = True +# or +c.Authenticator.allowed_users = {"name", ...} +``` + +::: + +:::{versionchanged} 5.0 +If no allow config is specified, then by default **nobody will have access to your Hub**. +Prior to 5.0, the opposite was true; effectively `allow_all = True` if no other allow config was specified. +::: You can restrict which users are allowed to login with a set, `Authenticator.allowed_users`: ```python c.Authenticator.allowed_users = {'mal', 'zoe', 'inara', 'kaylee'} -c.Authenticator.allow_all = False +# c.Authenticator.allow_all = False c.Authenticator.allow_existing_users = False ``` Users in the `allowed_users` set are added to the Hub database when the Hub is started. -```{warning} -If `allowed_users` is not specified, then by default **all authenticated users will be allowed into your hub**, -i.e. `allow_all` defaults to True if neither `allowed_users` nor `allow_all` are set. -``` +:::{versionchanged} 5.0 +{attr}`.Authenticator.allow_all` and {attr}`.Authenticator.allow_existing_users` are new in JupyterHub 5.0 +to enable explicit configuration of previously implicit behavior. -:::{versionadded} 5.0 -{attr}`Authenticator.allow_all` and {attr}`Authenticator.allow_existing_users` are new in JupyterHub 5.0. +Prior to 5.0, `allow_all` was implicitly True if `allowed_users` was empty. +Starting with 5.0, to allow all authenticated users by default, +`allow_all` must be explicitly set to True. -By default, `allow_all` is True when `allowed_users` is empty, -and `allow_existing_users` is True when `allowed_users` is not empty. -This is to ensure backward-compatibility. +By default, `allow_existing_users` is True when `allowed_users` is not empty, +to ensure backward-compatibility. +To make the `allowed_users` set _restrictive_, +set `allow_existing_users = False`. ::: ## One Time Passwords ( request_otp ) @@ -102,6 +129,11 @@ By default, only the deprecated `admin` role has global `access` permissions. ## Add or remove users from the Hub +:::{versionadded} 5.0 +`c.Authenticator.allow_existing_users` is added in 5.0 and enabled by default. +Prior to 5.0, this behavior was not optional. +::: + Users can be added to and removed from the Hub via the admin panel or the REST API. diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 01380b42..97090652 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2077,6 +2077,9 @@ class JupyterHub(Application): "auth_state is enabled, but encryption is not available: %s" % e ) + # give the authenticator a chance to check its own config + self.authenticator.check_allow_config() + if self.admin_users and not self.authenticator.admin_users: self.log.warning( "\nJupyterHub.admin_users is deprecated since version 0.7.2." @@ -2104,9 +2107,9 @@ class JupyterHub(Application): new_users.append(user) else: user.admin = True + # the admin_users config variable will never be used after this point. # only the database values will be referenced. - allowed_users = [ self.authenticator.normalize_username(name) for name in self.authenticator.allowed_users @@ -2116,10 +2119,10 @@ class JupyterHub(Application): if not self.authenticator.validate_username(username): raise ValueError("username %r is not valid" % username) - if not allowed_users: - self.log.info( - "Not using allowed_users. Any authenticated user will be allowed." - ) + if self.authenticator.allowed_users and self.authenticator.admin_users: + # make sure admin users are in the allowed_users set, if defined, + # otherwise they won't be able to login + self.authenticator.allowed_users |= self.authenticator.admin_users # add allowed users to the db for name in allowed_users: diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 70092fd9..312cc06c 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -121,6 +121,54 @@ class Authenticator(LoggingConfigurable): """ ).tag(config=True) + any_allow_config = Bool( + False, + help="""Is there any allow config? + + Used to show a warning if it looks like nobody can access the HUb, + which can happen when upgrading to JupyterHub 5, + now that `allow_all` defaults to False. + + Deployments can set this explicitly to True to suppress + the "No allow config found" warning. + + Will be True if any config tagged with `.tag(allow_config=True)` + or starts with `allow` is truthy. + + .. versionadded:: 5.0 + """, + ).tag(config=True) + + @default("any_allow_config") + def _default_any_allowed(self): + for trait_name, trait in self.traits(config=True).items(): + if trait.metadata.get("allow_config", False) or trait_name.startswith( + "allow" + ): + # this is only used for a helpful warning, so not the biggest deal if it's imperfect + if getattr(self, trait_name): + return True + return False + + def check_allow_config(self): + """Log a warning if no allow config can be found. + + Could get a false positive if _only_ unrecognized allow config is used. + Authenticators can apply `.tag(allow_config=True)` to label this config + to make sure it is found. + + Subclasses can override to perform additonal checks and warn about likely + authenticator configuration problems. + + .. versionadded:: 5.0 + """ + if not self.any_allow_config: + self.log.warning( + "No allow config found, it's possible that nobody can login to your Hub!\n" + "You can set `c.Authenticator.allow_all = True` to allow any user who can login to access the Hub,\n" + "or e.g. `allowed_users` to a set of users who should have access." + ) + whitelist = Set( help="Deprecated, use `Authenticator.allowed_users`", config=True, @@ -145,50 +193,35 @@ class Authenticator(LoggingConfigurable): ).tag(config=True) allow_all = Bool( - # dynamic default computed from allowed_users + False, config=True, help=""" - Any users who can successfully authenticate are allowed to access JupyterHub. - - For backward-compatibility, this is True by default if `allowed_users` is unspecified, - False if `allowed_users` is specified. + Allow every user who can successfully authenticate to access JupyterHub. + + False by default, which means for most Authenticators, + _some_ allow-related configuration is required to allow any users to log in. Authenticator subclasses may override the default with e.g.:: @default("allow_all") def _default_allow_all(self): - return False - - # or - - @default("allow_all") - def _default_allow_all(self): - if self.allowed_users or self.allowed_groups: + # if _any_ auth config (depends on the Authenticator) + if self.allowed_users or self.allowed_groups or self.allow_existing_users: return False else: return True - False is a good idea when the group of users who can authenticate is typically larger - than the group users who should have access (e.g. OAuth providers). - - Authenticator subclasses that define additional sources of `allow` config - beyond `allowed_users` should specify a default value for allow_all, - either always False or `if not any allow_config`. - - This is checked inside `check_allowed`, so subclasses that override `check_allowed` - must explicitly check `allow_all` for it to have any effect. - This is for safety, to ensure that no Authenticator subclass gets unexpected behavior from `allow_all`. - .. versionadded:: 5.0 + + .. versionchanged:: 5.0 + Prior to 5.0, `allow_all` wasn't defined on its own, + and was instead implicitly True when no allow config was provided, + i.e. `allowed_users` unspecified or empty on the base Authenticator class. + + To preserve pre-5.0 behavior, + set `allow_all = True` if you have no other allow configuration. """, - ) - - @default("allow_all") - def _default_allow_all(self): - if self.allowed_users: - return False - else: - return True + ).tag(allow_config=True) allow_existing_users = Bool( # dynamic default computed from allowed_users @@ -223,7 +256,7 @@ class Authenticator(LoggingConfigurable): .. versionadded:: 5.0 """, - ) + ).tag(allow_config=True) @default("allow_existing_users") def _allow_existing_users_default(self): @@ -616,8 +649,9 @@ class Authenticator(LoggingConfigurable): The various stages can be overridden separately: - `authenticate` turns formdata into a username - `normalize_username` normalizes the username - - `check_allowed` checks against the allowed usernames - `check_blocked_users` check against the blocked usernames + - `allow_all` is checked + - `check_allowed` checks against the allowed usernames - `is_admin` check if a user is an admin .. versionchanged:: 0.8 @@ -651,7 +685,11 @@ class Authenticator(LoggingConfigurable): self.log.warning("User %r blocked. Stop authentication", username) return - allowed_pass = await maybe_future(self.check_allowed(username, authenticated)) + allowed_pass = self.allow_all + if not allowed_pass: + allowed_pass = await maybe_future( + self.check_allowed(username, authenticated) + ) if allowed_pass: if authenticated['admin'] is None: @@ -776,14 +814,14 @@ class Authenticator(LoggingConfigurable): By default, this adds the user to the allowed_users set if allow_existing_users is true. - Subclasses may do more extensive things, such as adding actual unix users, + Subclasses may do more extensive things, such as creating actual system users, but they should call super to ensure the allowed_users set is updated. Note that this should be idempotent, since it is called whenever the hub restarts for all users. .. versionchanged:: 5.0 - Now adds users to the allowed_users set if allow_all is False, + Now adds users to the allowed_users set if allow_all is False and allow_existing_users is True, instead of if allowed_users is not empty. Args: @@ -791,10 +829,7 @@ class Authenticator(LoggingConfigurable): """ if not self.validate_username(user.name): raise ValueError("Invalid username: %s" % user.name) - # this is unnecessary if allow_all is True, - # but skipping this when allow_all is False breaks backward-compatibility - # for Authenticator subclasses that may not yet understand allow_all - if self.allow_existing_users: + if self.allow_existing_users and not self.allow_all: self.allowed_users.add(user.name) def delete_user(self, user): @@ -1005,18 +1040,9 @@ class LocalAuthenticator(Authenticator): `allowed_groups` may be specified together with allowed_users, to grant access by group OR name. """ - ).tag(config=True) - - @default("allow_all") - def _allow_all_default(self): - if self.allowed_users or self.allowed_groups: - # if any allow config is specified, default to False - return False - return True + ).tag(config=True, allow_config=True) def check_allowed(self, username, authentication=None): - if self.allow_all: - return True if self.check_allowed_groups(username, authentication): return True return super().check_allowed(username, authentication) @@ -1349,8 +1375,20 @@ class DummyAuthenticator(Authenticator): if it logs in with that password. .. versionadded:: 1.0 + + .. versionadded:: 5.0 + `allow_all` defaults to True, + preserving default behavior. """ + @default("allow_all") + def _allow_all_default(self): + if self.allowed_users: + return False + else: + # allow all by default + return True + password = Unicode( config=True, help=""" @@ -1360,6 +1398,12 @@ class DummyAuthenticator(Authenticator): """, ) + def check_allow_config(self): + super().check_allow_config() + self.log.warning( + f"Using testing authenticator {self.__class__.__name__}! This is not meant for production!" + ) + async def authenticate(self, handler, data): """Checks against a global password if it's been set. If not, allow any user/pass combo""" if self.password: diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index c4686829..e01c1e4b 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -243,6 +243,7 @@ class MockHub(JupyterHub): cert_location = kwargs['internal_certs_location'] kwargs['external_certs'] = ssl_setup(cert_location, 'hub-ca') super().__init__(*args, **kwargs) + self.config.Authenticator.allow_all = True @default('subdomain_host') def _subdomain_host_default(self): diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index a298283b..7cfda23d 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -19,7 +19,7 @@ from .utils import add_user, async_requests, get_page, public_url async def test_pam_auth(): - authenticator = MockPAMAuthenticator() + authenticator = MockPAMAuthenticator(allow_all=True) authorized = await authenticator.get_authenticated_user( None, {'username': 'match', 'password': 'match'} ) @@ -38,7 +38,7 @@ async def test_pam_auth(): async def test_pam_auth_account_check_disabled(): - authenticator = MockPAMAuthenticator(check_account=False) + authenticator = MockPAMAuthenticator(allow_all=True, check_account=False) authorized = await authenticator.get_authenticated_user( None, {'username': 'allowedmatch', 'password': 'allowedmatch'} ) @@ -83,7 +83,9 @@ async def test_pam_auth_admin_groups(): return user_group_map[name] authenticator = MockPAMAuthenticator( - admin_groups={'jh_admins', 'wheel'}, admin_users={'override_admin'} + admin_groups={'jh_admins', 'wheel'}, + admin_users={'override_admin'}, + allow_all=True, ) # Check admin_group applies as expected @@ -142,7 +144,10 @@ async def test_pam_auth_admin_groups(): async def test_pam_auth_allowed(): - authenticator = MockPAMAuthenticator(allowed_users={'wash', 'kaylee'}) + authenticator = MockPAMAuthenticator( + allowed_users={'wash', 'kaylee'}, allow_all=False + ) + authorized = await authenticator.get_authenticated_user( None, {'username': 'kaylee', 'password': 'kaylee'} ) @@ -163,7 +168,7 @@ async def test_pam_auth_allowed_groups(): def getgrnam(name): return MockStructGroup('grp', ['kaylee']) - authenticator = MockPAMAuthenticator(allowed_groups={'group'}) + authenticator = MockPAMAuthenticator(allowed_groups={'group'}, allow_all=False) with mock.patch.object(authenticator, '_getgrnam', getgrnam): authorized = await authenticator.get_authenticated_user( @@ -180,14 +185,14 @@ async def test_pam_auth_allowed_groups(): async def test_pam_auth_blocked(): # Null case compared to next case - authenticator = MockPAMAuthenticator() + authenticator = MockPAMAuthenticator(allow_all=True) authorized = await authenticator.get_authenticated_user( None, {'username': 'wash', 'password': 'wash'} ) assert authorized['name'] == 'wash' - # Blacklist basics - authenticator = MockPAMAuthenticator(blocked_users={'wash'}) + # Blocklist basics + authenticator = MockPAMAuthenticator(blocked_users={'wash'}, allow_all=True) authorized = await authenticator.get_authenticated_user( None, {'username': 'wash', 'password': 'wash'} ) @@ -195,7 +200,9 @@ async def test_pam_auth_blocked(): # User in both allowed and blocked: default deny. Make error someday? authenticator = MockPAMAuthenticator( - blocked_users={'wash'}, allowed_users={'wash', 'kaylee'} + blocked_users={'wash'}, + allowed_users={'wash', 'kaylee'}, + allow_all=True, ) authorized = await authenticator.get_authenticated_user( None, {'username': 'wash', 'password': 'wash'} @@ -204,7 +211,8 @@ async def test_pam_auth_blocked(): # User not in blocked set can log in authenticator = MockPAMAuthenticator( - blocked_users={'wash'}, allowed_users={'wash', 'kaylee'} + blocked_users={'wash'}, + allowed_users={'wash', 'kaylee'}, ) authorized = await authenticator.get_authenticated_user( None, {'username': 'kaylee', 'password': 'kaylee'} @@ -222,7 +230,8 @@ async def test_pam_auth_blocked(): # User in neither list authenticator = MockPAMAuthenticator( - blocked_users={'mal'}, allowed_users={'wash', 'kaylee'} + blocked_users={'mal'}, + allowed_users={'wash', 'kaylee'}, ) authorized = await authenticator.get_authenticated_user( None, {'username': 'simon', 'password': 'simon'} @@ -258,7 +267,9 @@ async def test_deprecated_signatures(): async def test_pam_auth_no_such_group(): - authenticator = MockPAMAuthenticator(allowed_groups={'nosuchcrazygroup'}) + authenticator = MockPAMAuthenticator( + allowed_groups={'nosuchcrazygroup'}, + ) authorized = await authenticator.get_authenticated_user( None, {'username': 'kaylee', 'password': 'kaylee'} ) @@ -406,7 +417,7 @@ async def test_auth_state_disabled(app, auth_state_unavailable): async def test_normalize_names(): - a = MockPAMAuthenticator() + a = MockPAMAuthenticator(allow_all=True) authorized = await a.get_authenticated_user( None, {'username': 'ZOE', 'password': 'ZOE'} ) @@ -429,7 +440,7 @@ async def test_normalize_names(): async def test_username_map(): - a = MockPAMAuthenticator(username_map={'wash': 'alpha'}) + a = MockPAMAuthenticator(username_map={'wash': 'alpha'}, allow_all=True) authorized = await a.get_authenticated_user( None, {'username': 'WASH', 'password': 'WASH'} ) @@ -459,7 +470,7 @@ async def test_post_auth_hook(): authentication['testkey'] = 'testvalue' return authentication - a = MockPAMAuthenticator(post_auth_hook=test_auth_hook) + a = MockPAMAuthenticator(allow_all=True, post_auth_hook=test_auth_hook) authorized = await a.get_authenticated_user( None, {'username': 'test_user', 'password': 'test_user'} @@ -567,6 +578,7 @@ async def test_auth_managed_groups( parent=app, authenticated_groups=authenticated_groups, refresh_groups=refresh_groups, + allow_all=True, ) user.groups.append(group) @@ -600,15 +612,18 @@ async def test_auth_managed_groups( "allowed_users, allow_all, allow_existing_users", [ ('specified', False, True), - ('', True, False), + ('', False, False), ], ) -def test_allow_all_defaults(app, user, allowed_users, allow_all, allow_existing_users): +async def test_allow_defaults( + app, user, allowed_users, allow_all, allow_existing_users +): if allowed_users: allowed_users = set(allowed_users.split(',')) else: allowed_users = set() authenticator = auth.Authenticator(allowed_users=allowed_users) + authenticator.authenticate = lambda handler, data: data["username"] assert authenticator.allow_all == allow_all assert authenticator.allow_existing_users == allow_existing_users @@ -620,8 +635,21 @@ def test_allow_all_defaults(app, user, allowed_users, allow_all, allow_existing_ else: authenticator.allowed_users == set() - assert authenticator.check_allowed("specified") - assert authenticator.check_allowed(user.name) + specified_allowed = await authenticator.get_authenticated_user( + None, {"username": "specified"} + ) + if "specified" in allowed_users: + assert specified_allowed is not None + else: + assert specified_allowed is None + + existing_allowed = await authenticator.get_authenticated_user( + None, {"username": user.name} + ) + if allow_existing_users: + assert existing_allowed is not None + else: + assert existing_allowed is None @pytest.mark.parametrize("allow_all", [None, True, False]) @@ -693,6 +721,9 @@ class AllowAllIgnoringAuthenticator(auth.Authenticator): allowed_letters = Tuple(config=True, help="Initial letters to allow") + def authenticate(self, handler, data): + return {"name": data["username"]} + def check_allowed(self, username, auth=None): if not self.allowed_users and not self.allowed_letters: # this subclass doesn't know about the JupyterHub 5 allow_all config @@ -707,7 +738,7 @@ class AllowAllIgnoringAuthenticator(auth.Authenticator): # allow_all is not recognized by Authenticator subclass # make sure it doesn't make anything more permissive, at least -@pytest.mark.parametrize("allow_all", [True, False, None]) +@pytest.mark.parametrize("allow_all", [True, False]) @pytest.mark.parametrize( "allowed_users, allowed_letters, allow_existing_users, allowed, not_allowed", [ @@ -720,7 +751,7 @@ class AllowAllIgnoringAuthenticator(auth.Authenticator): ("specified", "a,b", True, "specified,alice,bebe,existing", "other"), ], ) -def test_authenticator_without_allow_all( +async def test_authenticator_without_allow_all( app, allowed_users, allowed_letters, @@ -753,10 +784,14 @@ def test_authenticator_without_allow_all( expected_allowed = sorted(allowed) expected_not_allowed = sorted(not_allowed) to_check = list(chain(expected_allowed, expected_not_allowed)) + if allow_all: + expected_allowed = to_check + expected_not_allowed = [] + are_allowed = [] are_not_allowed = [] for username in to_check: - if authenticator.check_allowed(username): + if await authenticator.get_authenticated_user(None, {"username": username}): are_allowed.append(username) else: are_not_allowed.append(username) From 5831079bf65dfbc372fcd29d769ee9bb0cb5447f Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 22 Mar 2024 23:46:49 +0100 Subject: [PATCH 19/43] allow subclasses to override xsrf check need to inject our override into the base class, rather than at the instance level, to avoid clobbering any overrides in extensions like jupyter-server-proxy --- jupyterhub/services/auth.py | 44 ++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index b270f3b2..ac50e5c3 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -936,13 +936,39 @@ class HubOAuth(HubAuth): def _patch_xsrf(self, handler): """Patch handler to inject JuptyerHub xsrf token behavior""" - handler._xsrf_token_id = self._get_xsrf_token_id(handler) - # override xsrf_token property on class, - # so it's still a getter, not invoked immediately - handler.__class__.xsrf_token = property( - partial(get_xsrf_token, cookie_path=self.base_url) - ) - handler.check_xsrf_cookie = partial(self.check_xsrf_cookie, handler) + if isinstance(handler, HubAuthenticated): + # doesn't need patch + return + + # patch in our xsrf token handling + # overrides tornado and jupyter_server defaults, + # but not others. + # subclasses will still inherit our overridden behavior, + # but their overrides (if any) will take precedence over ours + # such as jupyter-server-proxy + for cls in handler.__class__.__mro__: + # search for the nearest parent class defined + # in one of the 'base' Handler-defining packages. + # In current implementations, this will + # generally be jupyter_server.base.handlers.JupyterHandler + # or tornado.web.RequestHandler, + # but doing it this way ensures consistent results + if (cls.__module__ or '').partition('.')[0] not in { + "jupyter_server", + "notebook", + "tornado", + }: + continue + # override check_xsrf_cookie where it's defined + if "check_xsrf_cookie" in cls.__dict__: + if "_get_xsrf_token_id" in cls.__dict__: + # already patched + return + cls._xsrf_token_id = property(self._get_xsrf_token_id) + cls.xsrf_token = property( + partial(get_xsrf_token, cookie_path=self.base_url) + ) + cls.check_xsrf_cookie = lambda handler: self.check_xsrf_cookie(handler) def check_xsrf_cookie(self, handler): """check_xsrf_cookie patch @@ -987,8 +1013,10 @@ class HubOAuth(HubAuth): token = self._get_token_cookie(handler) session_id = self.get_session_id(handler) if token and self._needs_check_xsrf(handler): + # call handler.check_xsrf_cookie instead of self.check_xsrf_cookie + # to allow subclass overrides try: - self.check_xsrf_cookie(handler) + handler.check_xsrf_cookie() except HTTPError as e: self.log.error( f"Not accepting cookie auth on {handler.request.method} {handler.request.path}: {e}" From f4aa8a4c25bc0e13ae47fa84907ffcb670c52e95 Mon Sep 17 00:00:00 2001 From: Min RK Date: Sat, 23 Mar 2024 16:03:30 +0100 Subject: [PATCH 20/43] changelog for 4.1.1 --- docs/source/reference/changelog.md | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index 0ffc2b1e..188c45be 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -10,7 +10,27 @@ command line for details. ## 4.1 -### 4.1.0 - 2024-03 +### 4.1.1 - 2024-03-23 + +4.1.1 fixes a compatibility regression in 4.1.0 for some extensions, +particularly jupyter-server-proxy. + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.0...4.1.1)) + +#### Bugs fixed + +- allow subclasses to override xsrf check [#4745](https://github.com/jupyterhub/jupyterhub/pull/4745) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2024-03-20&to=2024-03-23&type=c)) + +@consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2024-03-20..2024-03-23&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2024-03-20..2024-03-23&type=Issues)) + +### 4.1.0 - 2024-03-20 JupyterHub 4.1 is a security release, fixing [CVE-2024-28233]. All JupyterHub deployments are encouraged to upgrade, From b98af09df8b0e0904749901e97e45f48de8def40 Mon Sep 17 00:00:00 2001 From: Min RK Date: Sun, 24 Mar 2024 17:24:27 +0100 Subject: [PATCH 21/43] test: MockHub default allow_all=True not unconditional --- jupyterhub/tests/mocking.py | 3 ++- jupyterhub/tests/test_app.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index e01c1e4b..01bcf909 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -243,7 +243,8 @@ class MockHub(JupyterHub): cert_location = kwargs['internal_certs_location'] kwargs['external_certs'] = ssl_setup(cert_location, 'hub-ca') super().__init__(*args, **kwargs) - self.config.Authenticator.allow_all = True + if 'allow_all' not in self.config.Authenticator: + self.config.Authenticatorallow_all = True @default('subdomain_host') def _subdomain_host_default(self): diff --git a/jupyterhub/tests/test_app.py b/jupyterhub/tests/test_app.py index 9a3f0e2a..819a00e6 100644 --- a/jupyterhub/tests/test_app.py +++ b/jupyterhub/tests/test_app.py @@ -475,6 +475,7 @@ async def test_user_creation(tmpdir, request): ] cfg = Config() + cfg.Authenticator.allow_all = False cfg.Authenticator.allowed_users = allowed_users cfg.JupyterHub.load_groups = groups cfg.JupyterHub.load_roles = roles From f253cc46adce335570c08d2af47eb5612f17cd86 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 25 Mar 2024 22:31:02 +0100 Subject: [PATCH 22/43] typo in mock hub Co-authored-by: Erik Sundell --- jupyterhub/tests/mocking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index 01bcf909..8da6a802 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -244,7 +244,7 @@ class MockHub(JupyterHub): kwargs['external_certs'] = ssl_setup(cert_location, 'hub-ca') super().__init__(*args, **kwargs) if 'allow_all' not in self.config.Authenticator: - self.config.Authenticatorallow_all = True + self.config.Authenticator.allow_all = True @default('subdomain_host') def _subdomain_host_default(self): From 7e25dd15e66538ba9f2dff70ff3d7aa3f91a0cc4 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 26 Mar 2024 09:00:01 +0100 Subject: [PATCH 23/43] clarify externally managed group Co-authored-by: Erik Sundell --- jupyterhub/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 312cc06c..e0327415 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -249,7 +249,7 @@ class Authenticator(LoggingConfigurable): When this is enabled and you wish to remove access for one or more users previously allowed, you must make sure that they are removed from the jupyterhub database. This can be tricky to do - if you stop allowing a group of externally managed users for example. + if you stop allowing an externally managed group of users for example. With this enabled, JupyterHub admin users can visit `/hub/admin` or use JupyterHub's REST API to add and remove users to manage who can login. From 1feb3564c1dad6619e1d1fc909dcf100d44c1e53 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 26 Mar 2024 09:00:57 +0100 Subject: [PATCH 24/43] apply suggestions from code review Co-authored-by: Erik Sundell --- jupyterhub/auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index e0327415..99868561 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -808,6 +808,7 @@ class Authenticator(LoggingConfigurable): This is called: - When a user first authenticates, _after_ all allow and block checks have passed - When the hub restarts, for all users in the database (i.e. users previously allowed) + - When a user is added to the database, either via configuration or REST API This method may be a coroutine. From 7e56bf7e2cb7c127eab9d9eec3edd060075a937d Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 25 Mar 2024 12:10:07 +0100 Subject: [PATCH 25/43] rework handling of multiple xsrf tokens rather than attempting to clear multiple tokens (too complicated, breaks named servers) look for and accept first valid token have to do our own cookie parsing because existing cookie implementations only return a single value for each key and default to selecting the _least_ likely to be correct, according to RFCs. set updated xsrf cookie on login to avoid needing two requests to get the right cookie # Conflicts: # jupyterhub/tests/test_services_auth.py --- jupyterhub/_xsrf_utils.py | 138 ++++++++++++----------- jupyterhub/handlers/base.py | 14 ++- jupyterhub/services/auth.py | 27 ++++- jupyterhub/tests/browser/test_browser.py | 106 +++++++++++------ jupyterhub/tests/conftest.py | 2 - jupyterhub/tests/test_services_auth.py | 11 +- 6 files changed, 190 insertions(+), 108 deletions(-) diff --git a/jupyterhub/_xsrf_utils.py b/jupyterhub/_xsrf_utils.py index 805d7df8..e3ecdaca 100644 --- a/jupyterhub/_xsrf_utils.py +++ b/jupyterhub/_xsrf_utils.py @@ -10,11 +10,9 @@ in both Hub and single-user code import base64 import hashlib -from datetime import datetime, timedelta, timezone from http.cookies import SimpleCookie from tornado import web -from tornado.httputil import format_timestamp from tornado.log import app_log @@ -60,41 +58,76 @@ def _create_signed_value_urlsafe(handler, name, value): return base64.urlsafe_b64encode(signed_value).rstrip(b"=") -def _clear_invalid_xsrf_cookie(handler, cookie_path): +def _get_xsrf_token_cookie(handler): """ - Clear invalid XSRF cookie + Get the _valid_ XSRF token and id from Cookie - This may an old XSRF token, or one set on / by another application. - Because we cannot trust browsers or tornado to give us the more specific cookie, - try to clear _both_ on / and on our prefix, - then reload the page. + Returns (xsrf_token, xsrf_id) found in Cookies header. + + multiple xsrf cookies may be set on multiple paths; + + RFC 6265 states that they should be in order of more specific path to less, + but ALSO states that servers should never rely on order. + + Tornado (6.4) and stdlib (3.12) SimpleCookie explicitly use the _last_ value, + which means the cookie with the _least_ specific prefix will be used if more than one is present. + + Because we sign values, we can get the first valid cookie and not worry about order too much. + + This is simplified from tornado's HTTPRequest.cookies property + only looking for a single cookie. """ - expired = format_timestamp(datetime.now(timezone.utc) - timedelta(days=366)) - cookie = SimpleCookie() - cookie["_xsrf"] = "" - morsel = cookie["_xsrf"] - morsel["expires"] = expired - morsel["path"] = "/" - # use Set-Cookie directly, - # because tornado's set_cookie and clear_cookie use a single _dict_, - # so we can't clear a cookie on multiple paths and then set it - handler.add_header("Set-Cookie", morsel.OutputString(None)) - if cookie_path != "/": - # clear it multiple times! - morsel["path"] = cookie_path - handler.add_header("Set-Cookie", morsel.OutputString(None)) + if "Cookie" not in handler.request.headers: + return (None, None) - if ( - handler.request.method.lower() == "get" - and handler.request.headers.get("Sec-Fetch-Mode", "navigate") == "navigate" - ): - # reload current page because any subsequent set_cookie - # will cancel the clearing of the cookie - # this only makes sense on GET requests - handler.redirect(handler.request.uri) - # halt any other processing of the request - raise web.Finish() + for chunk in handler.request.headers["Cookie"].split(";"): + key = chunk.partition("=")[0].strip() + if key != "_xsrf": + # we are only looking for the _xsrf cookie + # ignore everything else + continue + + # use stdlib parsing to handle quotes, validation, etc. + try: + xsrf_token = SimpleCookie(chunk)[key].value.encode("ascii") + except (ValueError, KeyError): + continue + + xsrf_token_id = _get_signed_value_urlsafe(handler, "_xsrf", xsrf_token) + + if xsrf_token_id: + # only return if we found a _valid_ xsrf cookie + # otherwise, keep looking + return (xsrf_token, xsrf_token_id) + # no valid token found found + return (None, None) + + +def _set_xsrf_cookie(handler, xsrf_id, *, cookie_path="", authenticated=None): + """Set xsrf token cookie""" + xsrf_token = _create_signed_value_urlsafe(handler, "_xsrf", xsrf_id) + xsrf_cookie_kwargs = {} + xsrf_cookie_kwargs.update(handler.settings.get('xsrf_cookie_kwargs', {})) + xsrf_cookie_kwargs.setdefault("path", cookie_path) + if authenticated is None: + try: + current_user = handler.current_user + except Exception: + authenticated = False + else: + authenticated = bool(current_user) + if not authenticated: + # limit anonymous xsrf cookies to one hour + xsrf_cookie_kwargs.pop("expires", None) + xsrf_cookie_kwargs.pop("expires_days", None) + xsrf_cookie_kwargs["max_age"] = 3600 + app_log.info( + "Setting new xsrf cookie for %r %r", + xsrf_id, + xsrf_cookie_kwargs, + ) + handler.set_cookie("_xsrf", xsrf_token, **xsrf_cookie_kwargs) def get_xsrf_token(handler, cookie_path=""): @@ -110,23 +143,8 @@ def get_xsrf_token(handler, cookie_path=""): _set_cookie = False # the raw cookie is the token - xsrf_token = xsrf_cookie = handler.get_cookie("_xsrf") - if xsrf_token: - try: - xsrf_token = xsrf_token.encode("ascii") - except UnicodeEncodeError: - xsrf_token = None - - xsrf_id_cookie = _get_signed_value_urlsafe(handler, "_xsrf", xsrf_token) - if xsrf_cookie and not xsrf_id_cookie: - # we have a cookie, but it's invalid! - # handle possibility of _xsrf being set multiple times, - # e.g. on / and on /hub/ - # this will reload the page if it's a GET request - app_log.warning( - "Attempting to clear invalid _xsrf cookie %r", xsrf_cookie[:4] + "..." - ) - _clear_invalid_xsrf_cookie(handler, cookie_path) + xsrf_token, xsrf_id_cookie = _get_xsrf_token_cookie(handler) + cookie_token = xsrf_token # check the decoded, signed value for validity xsrf_id = handler._xsrf_token_id @@ -146,22 +164,16 @@ def get_xsrf_token(handler, cookie_path=""): _set_cookie = ( handler.request.headers.get("Sec-Fetch-Mode", "navigate") == "navigate" ) + if xsrf_id_cookie and not _set_cookie: + # if we aren't setting a cookie here but we got one, + # this means things probably aren't going to work + app_log.warning( + "Not accepting incorrect xsrf token id in cookie on %s", + handler.request.path, + ) if _set_cookie: - xsrf_cookie_kwargs = {} - xsrf_cookie_kwargs.update(handler.settings.get('xsrf_cookie_kwargs', {})) - xsrf_cookie_kwargs.setdefault("path", cookie_path) - if not handler.current_user: - # limit anonymous xsrf cookies to one hour - xsrf_cookie_kwargs.pop("expires", None) - xsrf_cookie_kwargs.pop("expires_days", None) - xsrf_cookie_kwargs["max_age"] = 3600 - app_log.info( - "Setting new xsrf cookie for %r %r", - xsrf_id, - xsrf_cookie_kwargs, - ) - handler.set_cookie("_xsrf", xsrf_token, **xsrf_cookie_kwargs) + _set_xsrf_cookie(handler, xsrf_id, cookie_path=cookie_path) handler._xsrf_token = xsrf_token return xsrf_token diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 8f866b12..da8863c4 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -24,7 +24,12 @@ from tornado.log import app_log from tornado.web import RequestHandler, addslash from .. import __version__, orm, roles, scopes -from .._xsrf_utils import _anonymous_xsrf_id, check_xsrf_cookie, get_xsrf_token +from .._xsrf_utils import ( + _anonymous_xsrf_id, + _set_xsrf_cookie, + check_xsrf_cookie, + get_xsrf_token, +) from ..metrics import ( PROXY_ADD_DURATION_SECONDS, PROXY_DELETE_DURATION_SECONDS, @@ -746,6 +751,13 @@ class BaseHandler(RequestHandler): self.log.info(f"User {cookie_user.name} is logging in as {user.name}") self.set_hub_cookie(user) + # make sure xsrf cookie is updated + # this avoids needing a second request to set the right xsrf cookie + self._jupyterhub_user = user + _set_xsrf_cookie( + self, self._xsrf_token_id, cookie_path=self.hub.base_url, authenticated=True + ) + def authenticate(self, data): return maybe_future(self.authenticator.get_authenticated_user(self, data)) diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index ac50e5c3..b038db58 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -44,6 +44,7 @@ from tornado.httpclient import AsyncHTTPClient, HTTPRequest from tornado.httputil import url_concat from tornado.log import app_log from tornado.web import HTTPError, RequestHandler +from tornado.websocket import WebSocketHandler from traitlets import ( Any, Bool, @@ -58,7 +59,12 @@ from traitlets import ( ) from traitlets.config import SingletonConfigurable -from .._xsrf_utils import _anonymous_xsrf_id, check_xsrf_cookie, get_xsrf_token +from .._xsrf_utils import ( + _anonymous_xsrf_id, + _set_xsrf_cookie, + check_xsrf_cookie, + get_xsrf_token, +) from ..scopes import _intersect_expanded_scopes from ..utils import _bool_env, get_browser_protocol, url_path_join @@ -856,6 +862,10 @@ class HubAuth(SingletonConfigurable): if not hasattr(self, 'set_cookie'): # only HubOAuth can persist cookies return + fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "navigate") + if isinstance(handler, WebSocketHandler) or fetch_mode != "navigate": + # don't do this on websockets or non-navigate requests + return self.log.info( "Storing token from url in cookie for %s", handler.request.remote_ip, @@ -907,6 +917,8 @@ class HubOAuth(HubAuth): def _get_token_cookie(self, handler): """Base class doesn't store tokens in cookies""" + if hasattr(handler, "_hub_auth_token_cookie"): + return handler._hub_auth_token_cookie fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unset") if fetch_mode == "websocket" and not self.allow_websocket_cookie_auth: @@ -1018,8 +1030,8 @@ class HubOAuth(HubAuth): try: handler.check_xsrf_cookie() except HTTPError as e: - self.log.error( - f"Not accepting cookie auth on {handler.request.method} {handler.request.path}: {e}" + self.log.debug( + f"Not accepting cookie auth on {handler.request.method} {handler.request.path}: {e.log_message}" ) # don't proceed with cookie auth unless xsrf is okay # don't raise either, because that makes a mess @@ -1270,6 +1282,15 @@ class HubOAuth(HubAuth): kwargs, ) handler.set_secure_cookie(self.cookie_name, access_token, **kwargs) + # set updated xsrf token cookie, + # which changes after login + handler._hub_auth_token_cookie = access_token + _set_xsrf_cookie( + handler, + handler._xsrf_token_id, + cookie_path=self.base_url, + authenticated=True, + ) def clear_cookie(self, handler): """Clear the OAuth cookie diff --git a/jupyterhub/tests/browser/test_browser.py b/jupyterhub/tests/browser/test_browser.py index 95a53952..b3821948 100644 --- a/jupyterhub/tests/browser/test_browser.py +++ b/jupyterhub/tests/browser/test_browser.py @@ -13,6 +13,7 @@ from tornado.escape import url_escape from tornado.httputil import url_concat from jupyterhub import orm, roles, scopes +from jupyterhub.tests.test_named_servers import named_servers # noqa from jupyterhub.tests.utils import async_requests, public_host, public_url, ujoin from jupyterhub.utils import url_escape_path, url_path_join @@ -1251,6 +1252,7 @@ async def test_start_stop_server_on_admin_page( "fresh", "invalid", "valid-prefix-invalid-root", + "valid-prefix-invalid-other-prefix", ], ) async def test_login_xsrf_initial_cookies(app, browser, case, username): @@ -1260,6 +1262,7 @@ async def test_login_xsrf_initial_cookies(app, browser, case, username): """ hub_root = public_host(app) hub_url = url_path_join(public_host(app), app.hub.base_url) + hub_parent = hub_url.rstrip("/").rsplit("/", 1)[0] + "/" login_url = url_path_join( hub_url, url_concat("login", {"next": url_path_join(app.base_url, "/hub/home")}) ) @@ -1269,7 +1272,11 @@ async def test_login_xsrf_initial_cookies(app, browser, case, username): await browser.context.add_cookies( [{"name": "_xsrf", "value": "invalid-hub-prefix", "url": hub_url}] ) - elif case == "valid-prefix-invalid-root": + elif case.startswith("valid-prefix"): + if "invalid-root" in case: + invalid_url = hub_root + else: + invalid_url = hub_parent await browser.goto(login_url) # first visit sets valid xsrf cookie cookies = await browser.context.cookies() @@ -1281,7 +1288,7 @@ async def test_login_xsrf_initial_cookies(app, browser, case, username): # currently, this test assumes the observed behavior, # which is that the invalid cookie on `/` has _higher_ priority await browser.context.add_cookies( - [{"name": "_xsrf", "value": "invalid-root", "url": hub_root}] + [{"name": "_xsrf", "value": "invalid-root", "url": invalid_url}] ) cookies = await browser.context.cookies() assert len(cookies) == 2 @@ -1314,7 +1321,14 @@ def _cookie_dict(cookie_list): return cookie_dict -async def test_singleuser_xsrf(app, browser, user, create_user_with_scopes, full_spawn): +async def test_singleuser_xsrf( + app, + browser, + user, + create_user_with_scopes, + full_spawn, + named_servers, # noqa: F811 +): # full login process, checking XSRF handling # start two servers target_user = user @@ -1435,33 +1449,61 @@ async def test_singleuser_xsrf(app, browser, user, create_user_with_scopes, full # check that server page can still connect to its own kernels token = target_user.new_api_token(scopes=["access:servers!user"]) - url = url_path_join(public_url(app, target_user), "/api/kernels") - headers = {"Authorization": f"Bearer {token}"} - r = await async_requests.post(url, headers=headers) - r.raise_for_status() - kernel = r.json() - kernel_id = kernel["id"] - kernel_url = url_path_join(url, kernel_id) - kernel_ws_url = "ws" + url_path_join(kernel_url, "channels")[4:] - try: - result = await browser.evaluate( - """ - async (ws_url) => { - ws = new WebSocket(ws_url); - finished = await new Promise((resolve, reject) => { - ws.onerror = (err) => { - reject(err); - }; - ws.onopen = () => { - resolve("ok"); - }; - }); - return finished; - } - """, - kernel_ws_url, - ) - finally: - r = await async_requests.delete(kernel_url, headers=headers) + + async def test_kernel(kernels_url): + headers = {"Authorization": f"Bearer {token}"} + r = await async_requests.post(kernels_url, headers=headers) r.raise_for_status() - assert result == "ok" + kernel = r.json() + kernel_id = kernel["id"] + kernel_url = url_path_join(kernels_url, kernel_id) + kernel_ws_url = "ws" + url_path_join(kernel_url, "channels")[4:] + try: + result = await browser.evaluate( + """ + async (ws_url) => { + ws = new WebSocket(ws_url); + finished = await new Promise((resolve, reject) => { + ws.onerror = (err) => { + reject(err); + }; + ws.onopen = () => { + resolve("ok"); + }; + }); + return finished; + } + """, + kernel_ws_url, + ) + finally: + r = await async_requests.delete(kernel_url, headers=headers) + r.raise_for_status() + assert result == "ok" + + kernels_url = url_path_join(public_url(app, target_user), "/api/kernels") + await test_kernel(kernels_url) + + # final check: make sure named servers work. + # first, visit spawn page to launch server, + # will issue cookies, etc. + server_name = "named" + url = url_path_join( + public_host(app), + url_path_join(app.base_url, f"hub/spawn/{browser_user.name}/{server_name}"), + ) + await browser.goto(url) + await expect(browser).to_have_url( + re.compile(rf".*/user/{browser_user.name}/{server_name}/.*") + ) + # from named server URL, make sure we can talk to a kernel + token = browser_user.new_api_token(scopes=["access:servers!user"]) + # named-server URL + kernels_url = url_path_join( + public_url(app, browser_user), server_name, "api/kernels" + ) + await test_kernel(kernels_url) + # go back to user's own page, test again + # make sure we didn't break anything + await browser.goto(public_url(app, browser_user)) + await test_kernel(url_path_join(public_url(app, browser_user), "api/kernels")) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index b0f215fc..a77c5d66 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -478,8 +478,6 @@ def create_user_with_scopes(app, create_temp_role): return app.users[orm_user.id] yield temp_user_creator - for user in temp_users: - app.users.delete(user) @fixture diff --git a/jupyterhub/tests/test_services_auth.py b/jupyterhub/tests/test_services_auth.py index e5aa3349..b45ba921 100644 --- a/jupyterhub/tests/test_services_auth.py +++ b/jupyterhub/tests/test_services_auth.py @@ -552,9 +552,8 @@ async def test_oauth_cookie_collision( else: raise ValueError(f"finish_first should be 1 or 2, not {finish_first!r}") # submit the oauth form to complete authorization - r = await s.post( - oauth.url, data={'scopes': ['identify'], "_xsrf": s.cookies["_xsrf"]} - ) + hub_xsrf = s.cookies.get("_xsrf", path=app.hub.base_url) + r = await s.post(oauth.url, data={'scopes': ['identify'], "_xsrf": hub_xsrf}) r.raise_for_status() assert r.url == expected_url # after finishing, state cookies are all cleared @@ -570,9 +569,7 @@ async def test_oauth_cookie_collision( assert service_cookie # finish other oauth - r = await s.post( - second_oauth.url, data={'scopes': ['identify'], "_xsrf": s.cookies["_xsrf"]} - ) + r = await s.post(second_oauth.url, data={'scopes': ['identify'], "_xsrf": hub_xsrf}) r.raise_for_status() # second oauth doesn't complete, @@ -638,7 +635,7 @@ async def test_oauth_logout(app, mockservice_url, create_user_with_scopes): r = await s.get(public_url(app, path='hub/logout')) r.raise_for_status() # verify that all cookies other than the service cookie are cleared - assert sorted(s.cookies.keys()) == ["_xsrf", service_cookie_name] + assert sorted(set(s.cookies.keys())) == ["_xsrf", service_cookie_name] # verify that clearing session id invalidates service cookie # i.e. redirect back to login page r = await s.get(url) From c08b582c53a72ba1c0e462c27d9715c10d1e8f1e Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 26 Mar 2024 08:55:15 +0100 Subject: [PATCH 26/43] respect jupyter-server disable_check_xsrf setting allows global disable of xsrf checks in single-user servers --- jupyterhub/services/auth.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index b038db58..fd322be1 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -987,7 +987,9 @@ class HubOAuth(HubAuth): Applies JupyterHub check_xsrf_cookie if not token authenticated """ - if getattr(handler, '_token_authenticated', False): + if getattr(handler, '_token_authenticated', False) or handler.settings.get( + "disable_check_xsrf", False + ): return check_xsrf_cookie(handler) From 2262bab4423feff984c43f529ec174bb40e0a1fb Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 25 Mar 2024 20:39:47 +0100 Subject: [PATCH 27/43] changelog for 4.1.2 --- docs/source/reference/changelog.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index dd88fa72..db49a438 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -22,6 +22,25 @@ Contributors to major version bumps in JupyterHub include: ## 4.1 +### 4.1.2 - 2024-03-25 + +4.1.2 fixes a regression in 4.1.0 affecting named servers. + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.1...4.1.2)) + +#### Bugs fixed + +- rework handling of multiple xsrf tokens [#4750](https://github.com/jupyterhub/jupyterhub/pull/4750) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2024-03-23&to=2024-03-25&type=c)) + +@consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2024-03-23..2024-03-25&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2024-03-23..2024-03-25&type=Issues)) + ### 4.1.1 - 2024-03-23 4.1.1 fixes a compatibility regression in 4.1.0 for some extensions, From 7d720371c5f061a4ff884ae98b41245de4cf3491 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 26 Mar 2024 09:44:07 +0100 Subject: [PATCH 28/43] changelog for 4.1.3 --- docs/source/reference/changelog.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index db49a438..b0ab8aa3 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -22,6 +22,23 @@ Contributors to major version bumps in JupyterHub include: ## 4.1 +### 4.1.3 - 2024-03-26 + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.2...4.1.3)) + +#### Bugs fixed + +- respect jupyter-server disable_check_xsrf setting [#4753](https://github.com/jupyterhub/jupyterhub/pull/4753) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2024-03-25&to=2024-03-26&type=c)) + +@consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2024-03-25..2024-03-26&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2024-03-25..2024-03-26&type=Issues)) + ### 4.1.2 - 2024-03-25 4.1.2 fixes a regression in 4.1.0 affecting named servers. From 9009bf2825f11dc02f7cde6a1274c569f9eed603 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 29 Mar 2024 10:03:51 +0000 Subject: [PATCH 29/43] Bump express from 4.18.2 to 4.19.2 in /jsx Bumps [express](https://github.com/expressjs/express) from 4.18.2 to 4.19.2. - [Release notes](https://github.com/expressjs/express/releases) - [Changelog](https://github.com/expressjs/express/blob/master/History.md) - [Commits](https://github.com/expressjs/express/compare/4.18.2...4.19.2) --- updated-dependencies: - dependency-name: express dependency-type: indirect ... Signed-off-by: dependabot[bot] --- jsx/package-lock.json | 57 ++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/jsx/package-lock.json b/jsx/package-lock.json index b8485724..7c43c26a 100644 --- a/jsx/package-lock.json +++ b/jsx/package-lock.json @@ -3599,12 +3599,13 @@ } }, "node_modules/body-parser": { - "version": "1.20.1", + "version": "1.20.2", + "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-1.20.2.tgz", + "integrity": "sha512-ml9pReCu3M61kGlqoTm2umSXTlRTuGTx0bfYj+uIUKKYycG5NtSbeetV3faSU6R7ajOPw0g/J1PvK4qNy7s5bA==", "dev": true, - "license": "MIT", "dependencies": { "bytes": "3.1.2", - "content-type": "~1.0.4", + "content-type": "~1.0.5", "debug": "2.6.9", "depd": "2.0.0", "destroy": "1.2.0", @@ -3612,7 +3613,7 @@ "iconv-lite": "0.4.24", "on-finished": "2.4.1", "qs": "6.11.0", - "raw-body": "2.5.1", + "raw-body": "2.5.2", "type-is": "~1.6.18", "unpipe": "1.0.0" }, @@ -3623,16 +3624,18 @@ }, "node_modules/body-parser/node_modules/debug": { "version": "2.6.9", + "resolved": "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz", + "integrity": "sha512-bC7ElrdJaJnPbAP+1EotYvqZsb3ecl5wi6Bfi6BJTUcNowp6cvspg0jXznRTKDjm/E7AdgFBVeAPVMNcKGsHMA==", "dev": true, - "license": "MIT", "dependencies": { "ms": "2.0.0" } }, "node_modules/body-parser/node_modules/iconv-lite": { "version": "0.4.24", + "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.4.24.tgz", + "integrity": "sha512-v3MXnZAcvnywkTUEZomIActle7RXXeedOR31wwl7VlyoXO4Qi9arvSenNQWne1TcRwhCL1HwLI21bEqdpj8/rA==", "dev": true, - "license": "MIT", "dependencies": { "safer-buffer": ">= 2.1.2 < 3" }, @@ -3642,8 +3645,9 @@ }, "node_modules/body-parser/node_modules/ms": { "version": "2.0.0", - "dev": true, - "license": "MIT" + "resolved": "https://registry.npmjs.org/ms/-/ms-2.0.0.tgz", + "integrity": "sha512-Tpp60P6IUJDTuOq/5Z8cdskzJujfwqfOTkrwIwj7IRISpnkJnT6SyJ4PCPnGMoFjC9ddhal5KVIYtAt97ix05A==", + "dev": true }, "node_modules/bonjour-service": { "version": "1.1.1", @@ -3739,8 +3743,9 @@ }, "node_modules/bytes": { "version": "3.1.2", + "resolved": "https://registry.npmjs.org/bytes/-/bytes-3.1.2.tgz", + "integrity": "sha512-/Nf7TyzTx6S3yRJObOAV7956r8cr2+Oj8AC5dt8wSP3BQAoeX58NoHyCU8P8zGkNXStjTSi6fzO6F0pBdcYbEg==", "dev": true, - "license": "MIT", "engines": { "node": ">= 0.8" } @@ -4051,8 +4056,9 @@ }, "node_modules/content-type": { "version": "1.0.5", + "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.5.tgz", + "integrity": "sha512-nTjqfcBFEipKdXCv4YDQWCfmcLZKm81ldF0pAopTvyrFGVbcR6P/VAAd5G7N+0tTr8QqiU0tFadD6FK4NtJwOA==", "dev": true, - "license": "MIT", "engines": { "node": ">= 0.6" } @@ -4063,9 +4069,10 @@ "license": "MIT" }, "node_modules/cookie": { - "version": "0.5.0", + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.6.0.tgz", + "integrity": "sha512-U71cyTamuh1CRNCfpGY6to28lxvNwPG4Guz/EVjgf3Jmzv0vlDp1atT9eS5dDjMYHucpHbWns6Lwf3BKz6svdw==", "dev": true, - "license": "MIT", "engines": { "node": ">= 0.6" } @@ -5071,16 +5078,17 @@ } }, "node_modules/express": { - "version": "4.18.2", + "version": "4.19.2", + "resolved": "https://registry.npmjs.org/express/-/express-4.19.2.tgz", + "integrity": "sha512-5T6nhjsT+EOMzuck8JjBHARTHfMht0POzlA60WV2pMD3gyXw2LZnZ+ueGdNxG+0calOJcWKbpFcuzLZ91YWq9Q==", "dev": true, - "license": "MIT", "dependencies": { "accepts": "~1.3.8", "array-flatten": "1.1.1", - "body-parser": "1.20.1", + "body-parser": "1.20.2", "content-disposition": "0.5.4", "content-type": "~1.0.4", - "cookie": "0.5.0", + "cookie": "0.6.0", "cookie-signature": "1.0.6", "debug": "2.6.9", "depd": "2.0.0", @@ -7319,8 +7327,9 @@ }, "node_modules/media-typer": { "version": "0.3.0", + "resolved": "https://registry.npmjs.org/media-typer/-/media-typer-0.3.0.tgz", + "integrity": "sha512-dq+qelQ9akHpcOl/gUVRTxVIOkAJ1wR3QAvb4RsVjS8oVoFjDGTc679wJYmUmknUF5HwMLOgb5O+a3KxfWapPQ==", "dev": true, - "license": "MIT", "engines": { "node": ">= 0.6" } @@ -8120,8 +8129,9 @@ }, "node_modules/qs": { "version": "6.11.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.11.0.tgz", + "integrity": "sha512-MvjoMCJwEarSbUYk5O+nmoSzSutSsTwF85zcHPQ9OrlFoZOYIjaqBAJIqIXjptyD5vThxGq52Xu/MaJzRkIk4Q==", "dev": true, - "license": "BSD-3-Clause", "dependencies": { "side-channel": "^1.0.4" }, @@ -8173,9 +8183,10 @@ } }, "node_modules/raw-body": { - "version": "2.5.1", + "version": "2.5.2", + "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.5.2.tgz", + "integrity": "sha512-8zGqypfENjCIqGhgXToC8aB2r7YrBX+AQAfIPs/Mlk+BtPTztOvTS01NRW/3Eh60J+a48lt8qsCzirQ6loCVfA==", "dev": true, - "license": "MIT", "dependencies": { "bytes": "3.1.2", "http-errors": "2.0.0", @@ -8188,8 +8199,9 @@ }, "node_modules/raw-body/node_modules/iconv-lite": { "version": "0.4.24", + "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.4.24.tgz", + "integrity": "sha512-v3MXnZAcvnywkTUEZomIActle7RXXeedOR31wwl7VlyoXO4Qi9arvSenNQWne1TcRwhCL1HwLI21bEqdpj8/rA==", "dev": true, - "license": "MIT", "dependencies": { "safer-buffer": ">= 2.1.2 < 3" }, @@ -9468,8 +9480,9 @@ }, "node_modules/type-is": { "version": "1.6.18", + "resolved": "https://registry.npmjs.org/type-is/-/type-is-1.6.18.tgz", + "integrity": "sha512-TkRKr9sUTxEH8MdfuCSP7VizJyzRNMjj2J2do2Jr3Kym598JVdEksuzPQCnlFPW4ky9Q+iA+ma9BGm06XQBy8g==", "dev": true, - "license": "MIT", "dependencies": { "media-typer": "0.3.0", "mime-types": "~2.1.24" From 26a0be5103e2d9d6f435179d84deebb316d166bc Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 29 Mar 2024 09:41:17 +0100 Subject: [PATCH 30/43] avoid xsrf check on navigate GET requests sevices/auth prevents calling check_xsrf_cookie, but if the Handler itself called it the newly strict check would still be applied this ensures the check is actually allowed for navigate GET requests --- jupyterhub/_xsrf_utils.py | 25 +++++++++++++++++++++++++ jupyterhub/services/auth.py | 23 ++--------------------- jupyterhub/tests/test_api.py | 2 +- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/jupyterhub/_xsrf_utils.py b/jupyterhub/_xsrf_utils.py index e3ecdaca..b93e2818 100644 --- a/jupyterhub/_xsrf_utils.py +++ b/jupyterhub/_xsrf_utils.py @@ -178,10 +178,35 @@ def get_xsrf_token(handler, cookie_path=""): return xsrf_token +def _needs_check_xsrf(handler): + """Does the given cookie-authenticated request need to check xsrf?""" + + if getattr(handler, "_token_authenticated", False): + return False + + fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unspecified") + if fetch_mode in {"websocket", "no-cors"} or ( + fetch_mode in {"navigate", "unspecified"} + and handler.request.method.lower() in {"get", "head", "options"} + ): + # no xsrf check needed for regular page views or no-cors + # or websockets after allow_websocket_cookie_auth passes + if fetch_mode == "unspecified": + app_log.warning( + f"Skipping XSRF check for insecure request {handler.request.method} {handler.request.path}" + ) + return False + else: + return True + + def check_xsrf_cookie(handler): """Check that xsrf cookie matches xsrf token in request""" # overrides tornado's implementation # because we changed what a correct value should be in xsrf_token + if not _needs_check_xsrf(handler): + # don't require XSRF for regular page views + return token = ( handler.get_argument("_xsrf", None) diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index fd322be1..503235ea 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -61,6 +61,7 @@ from traitlets.config import SingletonConfigurable from .._xsrf_utils import ( _anonymous_xsrf_id, + _needs_check_xsrf, _set_xsrf_cookie, check_xsrf_cookie, get_xsrf_token, @@ -1002,31 +1003,11 @@ class HubOAuth(HubAuth): kwargs["secure"] = True return handler.clear_cookie(cookie_name, **kwargs) - def _needs_check_xsrf(self, handler): - """Does the given cookie-authenticated request need to check xsrf?""" - if getattr(handler, "_token_authenticated", False): - return False - - fetch_mode = handler.request.headers.get("Sec-Fetch-Mode", "unspecified") - if fetch_mode in {"websocket", "no-cors"} or ( - fetch_mode in {"navigate", "unspecified"} - and handler.request.method.lower() in {"get", "head", "options"} - ): - # no xsrf check needed for regular page views or no-cors - # or websockets after allow_websocket_cookie_auth passes - if fetch_mode == "unspecified": - self.log.warning( - f"Skipping XSRF check for insecure request {handler.request.method} {handler.request.path}" - ) - return False - else: - return True - async def _get_user_cookie(self, handler): # check xsrf if needed token = self._get_token_cookie(handler) session_id = self.get_session_id(handler) - if token and self._needs_check_xsrf(handler): + if token and _needs_check_xsrf(handler): # call handler.check_xsrf_cookie instead of self.check_xsrf_cookie # to allow subclass overrides try: diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index ac81970c..77d16861 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -160,7 +160,7 @@ async def test_permission_error_messages(app, user, auth, expected_message): params["_xsrf"] = cookies["_xsrf"] if auth == "cookie_xsrf_mismatch": params["_xsrf"] = "somethingelse" - + headers['Sec-Fetch-Mode'] = 'cors' r = await async_requests.get(url, **kwargs) assert r.status_code == 403 response = r.json() From ab37cd7f249abd1cfba8d0cc4319963ae4965e0c Mon Sep 17 00:00:00 2001 From: Min RK Date: Sat, 30 Mar 2024 09:55:16 +0100 Subject: [PATCH 31/43] changelog for 4.1.4 --- docs/source/reference/changelog.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index b0ab8aa3..a6790893 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -22,6 +22,23 @@ Contributors to major version bumps in JupyterHub include: ## 4.1 +### 4.1.4 - 2024-03-30 + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.3...4.1.4)) + +#### Bugs fixed + +- avoid xsrf check on navigate GET requests [#4759](https://github.com/jupyterhub/jupyterhub/pull/4759) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2024-03-26&to=2024-03-30&type=c)) + +@consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2024-03-26..2024-03-30&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2024-03-26..2024-03-30&type=Issues)) + ### 4.1.3 - 2024-03-26 ([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.2...4.1.3)) From b678236f8704ecf8550436e22c3c1c2c562ed3fa Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 1 Apr 2024 22:10:17 +0000 Subject: [PATCH 32/43] [pre-commit.ci] pre-commit autoupdate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/astral-sh/ruff-pre-commit: v0.3.2 → v0.3.5](https://github.com/astral-sh/ruff-pre-commit/compare/v0.3.2...v0.3.5) --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 7bd24ba7..3d5e2a81 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -16,7 +16,7 @@ ci: repos: # autoformat and lint Python code - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.3.2 + rev: v0.3.5 hooks: - id: ruff types_or: From 587e6cec4ee2d1d73a641c2542e186d4d53ca901 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 2 Apr 2024 10:24:17 +0200 Subject: [PATCH 33/43] Fix typo in docstring about Authenticator.blocked_users --- jupyterhub/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 6a1f01a8..118ceb53 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -478,7 +478,7 @@ class Authenticator(LoggingConfigurable): return username in self.allowed_users def check_blocked_users(self, username, authentication=None): - """Check if a username is blocked to authenticate based on Authenticator.blocked configuration + """Check if a username is blocked to authenticate based on Authenticator.blocked_users configuration Return True if username is allowed, False otherwise. No block list means any username is allowed. From 3e61e4649795c9d5d04902471da78e628e9de20e Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 2 Apr 2024 10:50:31 +0200 Subject: [PATCH 34/43] Apply suggestions from code review Co-authored-by: Simon Li --- docs/source/reference/authenticators.md | 16 ++++++---------- jupyterhub/auth.py | 4 ++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/docs/source/reference/authenticators.md b/docs/source/reference/authenticators.md index 8c662c4d..7b395ddc 100644 --- a/docs/source/reference/authenticators.md +++ b/docs/source/reference/authenticators.md @@ -211,7 +211,7 @@ authorization It is perfectly fine in the simplest cases for `Authenticator.authenticate` to be responsible for authentication _and_ authorization, in which case `authenticate` may return `None` if the user is not authorized. -However, Authenticators also have have two methods {meth}`~.Authenticator.check_allowed` and {meth}`~.Authenticator.check_blocked_users`, which are called after successful authentication to further check if the user is allowed. +However, Authenticators also have two methods, {meth}`~.Authenticator.check_allowed` and {meth}`~.Authenticator.check_blocked_users`, which are called after successful authentication to further check if the user is allowed. If `check_blocked_users()` returns False, authorization stops and the user is not allowed. @@ -227,7 +227,7 @@ which is a change from pre-5.0, where `allow_all` was implicitly True if `allowe ### Overriding `check_allowed` :::{versionchanged} 5.0 -`check_allowed()` is **not called** is `allow_all` is True. +`check_allowed()` is **not called** if `allow_all` is True. ::: :::{versionchanged} 5.0 @@ -242,14 +242,10 @@ The base implementation of {meth}`~.Authenticator.check_allowed` checks: - else return False :::{versionchanged} 5.0 -Prior to 5.0, the check was +Prior to 5.0, this would also return True if `allowed_users` was empty. -```python -if (not allowed_users) or username in allowed_users: -``` - -but the implicit `not allowed_users` has been replaced by explicit `allow_all`, which is checked _before_ calling `check_allowed`. -`check_allowed` **is not called** if `allow_all` is True. +For clarity, this is no longer the case. A new `allow_all` property (default False) has been added which is checked _before_ calling `check_allowed`. +If `allow_all` is True, this takes priority over `check_allowed`, which will be ignored. If your Authenticator subclass similarly returns True when no allow config is defined, this is fully backward compatible for your users, but means `allow_all = False` has no real effect. @@ -349,7 +345,7 @@ So the logical expression for a user being authorized should look like: #### Custom error messages -Any of these authentication and authorization methods may +Any of these authentication and authorization methods may raise a `web.HTTPError` Exception ```python from tornado import web diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 99868561..aa200a51 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -125,7 +125,7 @@ class Authenticator(LoggingConfigurable): False, help="""Is there any allow config? - Used to show a warning if it looks like nobody can access the HUb, + Used to show a warning if it looks like nobody can access the Hub, which can happen when upgrading to JupyterHub 5, now that `allow_all` defaults to False. @@ -199,7 +199,7 @@ class Authenticator(LoggingConfigurable): Allow every user who can successfully authenticate to access JupyterHub. False by default, which means for most Authenticators, - _some_ allow-related configuration is required to allow any users to log in. + _some_ allow-related configuration is required to allow users to log in. Authenticator subclasses may override the default with e.g.:: From 3ce2643ab73b7a72e790e70f536e6d4d1225c538 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 2 Apr 2024 10:54:17 +0200 Subject: [PATCH 35/43] clarify some allow docs per review --- .../tutorial/getting-started/authenticators-users-basics.md | 3 ++- jupyterhub/auth.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/source/tutorial/getting-started/authenticators-users-basics.md b/docs/source/tutorial/getting-started/authenticators-users-basics.md index 6726091e..9c0c09eb 100644 --- a/docs/source/tutorial/getting-started/authenticators-users-basics.md +++ b/docs/source/tutorial/getting-started/authenticators-users-basics.md @@ -130,7 +130,8 @@ By default, only the deprecated `admin` role has global `access` permissions. ## Add or remove users from the Hub :::{versionadded} 5.0 -`c.Authenticator.allow_existing_users` is added in 5.0 and enabled by default. +`c.Authenticator.allow_existing_users` is added in 5.0 and True by default _if_ any `allowed_users` are specified. + Prior to 5.0, this behavior was not optional. ::: diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index aa200a51..4462f73a 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -166,7 +166,8 @@ class Authenticator(LoggingConfigurable): self.log.warning( "No allow config found, it's possible that nobody can login to your Hub!\n" "You can set `c.Authenticator.allow_all = True` to allow any user who can login to access the Hub,\n" - "or e.g. `allowed_users` to a set of users who should have access." + "or e.g. `allowed_users` to a set of users who should have access.\n" + "You may suppress this warning by setting c.Authenticator.any_allow_config = True." ) whitelist = Set( From 5424108593f6d86693ca4233a77d9452585e42a9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 4 Apr 2024 11:05:10 +0200 Subject: [PATCH 36/43] singleuser mixin: include check_xsrf_cookie in overrides --- jupyterhub/singleuser/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/singleuser/mixins.py b/jupyterhub/singleuser/mixins.py index c08fda9d..c1c60f9c 100755 --- a/jupyterhub/singleuser/mixins.py +++ b/jupyterhub/singleuser/mixins.py @@ -827,7 +827,7 @@ def patch_base_handler(BaseHandler, log=None): # but we also need to ensure BaseHandler *itself* doesn't # override the public tornado API methods we have inserted. # If they are defined in BaseHandler, explicitly replace them with our methods. - for name in ("get_current_user", "get_login_url"): + for name in ("get_current_user", "get_login_url", "check_xsrf_cookie"): if name in BaseHandler.__dict__: log.debug( f"Overriding {BaseHandler}.{name} with HubAuthenticatedHandler.{name}" From 47f39e7c2ff23f6854492e7f7a56cc4f0d07a704 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 4 Apr 2024 12:51:26 +0200 Subject: [PATCH 37/43] changelog for 4.1.5 --- docs/source/reference/changelog.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index a6790893..9ad3a112 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -22,6 +22,23 @@ Contributors to major version bumps in JupyterHub include: ## 4.1 +### 4.1.5 - 2024-04-04 + +([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.4...4.1.5)) + +#### Bugs fixed + +- singleuser mixin: include check_xsrf_cookie in overrides [#4771](https://github.com/jupyterhub/jupyterhub/pull/4771) ([@minrk](https://github.com/minrk), [@consideRatio](https://github.com/consideRatio)) + +#### Contributors to this release + +The following people contributed discussions, new ideas, code and documentation contributions, and review. +See [our definition of contributors](https://github-activity.readthedocs.io/en/latest/#how-does-this-tool-define-contributions-in-the-reports). + +([GitHub contributors page for this release](https://github.com/jupyterhub/jupyterhub/graphs/contributors?from=2024-03-30&to=2024-04-04&type=c)) + +@consideRatio ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3AconsideRatio+updated%3A2024-03-30..2024-04-04&type=Issues)) | @manics ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Amanics+updated%3A2024-03-30..2024-04-04&type=Issues)) | @minrk ([activity](https://github.com/search?q=repo%3Ajupyterhub%2Fjupyterhub+involves%3Aminrk+updated%3A2024-03-30..2024-04-04&type=Issues)) + ### 4.1.4 - 2024-03-30 ([full changelog](https://github.com/jupyterhub/jupyterhub/compare/4.1.3...4.1.4)) From 88189d54d9187db35ab49839a116e192ee252818 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 10 Apr 2024 10:34:28 +0100 Subject: [PATCH 38/43] Add a test for `allow_unauthenticated_access` (xfail) --- jupyterhub/tests/test_singleuser.py | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/jupyterhub/tests/test_singleuser.py b/jupyterhub/tests/test_singleuser.py index f5b16763..d2d4be17 100644 --- a/jupyterhub/tests/test_singleuser.py +++ b/jupyterhub/tests/test_singleuser.py @@ -2,6 +2,7 @@ import os import sys +import warnings from contextlib import nullcontext from pathlib import Path from pprint import pprint @@ -291,6 +292,57 @@ async def test_notebook_dir( raise ValueError(f"No contents check for {notebook_dir=}") +@pytest.mark.parametrize("extension", [True, False]) +@pytest.mark.skipif(IS_JUPYVERSE, reason="jupyverse has no auth configuration") +async def test_forbid_unauthenticated_access( + request, app, tmp_path, user, full_spawn, extension +): + try: + from jupyter_server.auth.decorator import allow_unauthenticated # noqa + except ImportError: + pytest.skip("needs jupyter-server 2.13") + + from jupyter_server.utils import JupyterServerAuthWarning + + # login, start the server + cookies = await app.login_user('nandy') + s = AsyncSession() + s.cookies = cookies + user = app.users['nandy'] + # stop spawner, if running: + if user.running: + await user.stop() + # start with new config: + user.spawner.default_url = "/jupyterhub-test-info" + + if extension: + user.spawner.environment["JUPYTERHUB_SINGLEUSER_EXTENSION"] = "1" + else: + user.spawner.environment["JUPYTERHUB_SINGLEUSER_EXTENSION"] = "0" + + # make sure it's resolved to start + tmp_path = tmp_path.resolve() + real_home_dir = tmp_path / "realhome" + real_home_dir.mkdir() + # make symlink to test resolution + home_dir = tmp_path / "home" + home_dir.symlink_to(real_home_dir) + # home_dir is defined on SimpleSpawner + user.spawner.home_dir = str(home_dir) + jupyter_config_dir = home_dir / ".jupyter" + jupyter_config_dir.mkdir() + # verify config paths + with (jupyter_config_dir / "jupyter_server_config.py").open("w") as f: + f.write("c.ServerApp.allow_unauthenticated_access = False") + + # If there are core endpoints (added by jupyterhub) without decorators, + # spawn will error out. If there are extension endpoints without decorators + # these will be logged as warnings. + with warnings.catch_warnings(): + warnings.simplefilter("error", JupyterServerAuthWarning) + await user.spawn() + + @pytest.mark.skipif(IS_JUPYVERSE, reason="jupyverse has no --help-all") def test_help_output(): out = check_output( From aefc8de49a77c92e7704ba57fba3e3a01af5fe21 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 10 Apr 2024 11:03:33 +0100 Subject: [PATCH 39/43] Add @allow_unauthenticated decorators --- jupyterhub/singleuser/_decorator.py | 14 ++++++++++++++ jupyterhub/singleuser/extension.py | 6 ++++++ jupyterhub/singleuser/mixins.py | 6 ++++++ 3 files changed, 26 insertions(+) create mode 100644 jupyterhub/singleuser/_decorator.py diff --git a/jupyterhub/singleuser/_decorator.py b/jupyterhub/singleuser/_decorator.py new file mode 100644 index 00000000..71f8c89f --- /dev/null +++ b/jupyterhub/singleuser/_decorator.py @@ -0,0 +1,14 @@ +from typing import Any, Callable, TypeVar + +try: + from jupyter_server.auth.decorator import allow_unauthenticated +except ImportError: + FuncT = TypeVar("FuncT", bound=Callable[..., Any]) + + # if using an older jupyter-server version this can be a no-op, + # as these do not support marking endpoints anyways + def allow_unauthenticated(method: FuncT) -> FuncT: + return method + + +__all__ = ["allow_unauthenticated"] diff --git a/jupyterhub/singleuser/extension.py b/jupyterhub/singleuser/extension.py index dc9e5f36..e34e05fc 100644 --- a/jupyterhub/singleuser/extension.py +++ b/jupyterhub/singleuser/extension.py @@ -51,6 +51,7 @@ from jupyterhub.utils import ( url_path_join, ) +from ._decorator import allow_unauthenticated from ._disable_user_config import _disable_user_config SINGLEUSER_TEMPLATES_DIR = str(Path(__file__).parent.resolve().joinpath("templates")) @@ -68,6 +69,7 @@ def _exclude_home(path_list): class JupyterHubLogoutHandler(LogoutHandler): + @allow_unauthenticated def get(self): hub_auth = self.identity_provider.hub_auth # clear token stored in single-user cookie (set by hub_auth) @@ -95,6 +97,10 @@ class JupyterHubOAuthCallbackHandler(HubOAuthCallbackHandler): def initialize(self, hub_auth): self.hub_auth = hub_auth + @allow_unauthenticated + async def get(self): + return await super().get() + class JupyterHubIdentityProvider(IdentityProvider): """Identity Provider for JupyterHub OAuth diff --git a/jupyterhub/singleuser/mixins.py b/jupyterhub/singleuser/mixins.py index c08fda9d..8496a6e0 100755 --- a/jupyterhub/singleuser/mixins.py +++ b/jupyterhub/singleuser/mixins.py @@ -52,6 +52,7 @@ from ..utils import ( make_ssl_context, url_path_join, ) +from ._decorator import allow_unauthenticated from ._disable_user_config import _disable_user_config, _exclude_home # Authenticate requests with the Hub @@ -132,6 +133,7 @@ class JupyterHubLoginHandlerMixin: class JupyterHubLogoutHandlerMixin: + @allow_unauthenticated def get(self): self.settings['hub_auth'].clear_cookie(self) self.redirect( @@ -147,6 +149,10 @@ class OAuthCallbackHandlerMixin(HubOAuthCallbackHandler): def hub_auth(self): return self.settings['hub_auth'] + @allow_unauthenticated + async def get(self): + return await super().get() + # register new hub related command-line aliases aliases = { From e3ea59759e819e2a802e9ecedb22a2223ba889f7 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 10 Apr 2024 12:56:41 +0200 Subject: [PATCH 40/43] clarify error template debug log 'No template for 404' looks like something's wrong, when all it means to convey is that it doesn't get _special_ treatment and the default error page is enough. --- jupyterhub/handlers/base.py | 2 +- jupyterhub/handlers/pages.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index da8863c4..b81189a7 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -1510,7 +1510,7 @@ class BaseHandler(RequestHandler): try: html = self.render_template('%s.html' % status_code, sync=True, **ns) except TemplateNotFound: - self.log.debug("No template for %d", status_code) + self.log.debug("Using default error template for %d", status_code) try: html = self.render_template('error.html', sync=True, **ns) except Exception: diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 97319d1c..ce26c44a 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -657,7 +657,7 @@ class ProxyErrorHandler(BaseHandler): try: html = await self.render_template('%s.html' % status_code, **ns) except TemplateNotFound: - self.log.debug("No template for %d", status_code) + self.log.debug("Using default error template for %d", status_code) html = await self.render_template('error.html', **ns) self.write(html) From 061d267d74e6c7435e83e8bac058292c5c3f1e15 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Fri, 12 Apr 2024 17:01:11 +0100 Subject: [PATCH 41/43] Token UI: move button to after form fields --- share/jupyterhub/templates/token.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/share/jupyterhub/templates/token.html b/share/jupyterhub/templates/token.html index 3f530734..e6f6163a 100644 --- a/share/jupyterhub/templates/token.html +++ b/share/jupyterhub/templates/token.html @@ -6,11 +6,6 @@

Manage JupyterHub Tokens

-
- -
JupyterHub documentation for a list of available scopes.
+
+ +
From 8fd09053a23cff9051f051022346660feb37f999 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 15 Apr 2024 10:20:10 +0200 Subject: [PATCH 42/43] avoid producing '[group]' string in output GitHub Actions starts a log expansion group when it sees the string `[group]`, which happens when that is a parametrize argument which results in collapsing all subsequent test outputs pytest.param lets us assign an id that is used in output, but not the value itself --- jupyterhub/tests/test_pages.py | 16 ++++++++++++---- jupyterhub/tests/test_shares.py | 6 ++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index c07441ca..6814cef0 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -213,7 +213,9 @@ async def test_spawn_handler_access(app): r.raise_for_status() -@pytest.mark.parametrize("has_access", ["all", "user", "group", False]) +@pytest.mark.parametrize( + "has_access", ["all", "user", (pytest.param("group", id="in-group")), False] +) async def test_spawn_other_user( app, user, username, group, create_temp_role, has_access ): @@ -300,7 +302,9 @@ async def test_spawn_page_falsy_callable(app): assert history[1] == ujoin(public_url(app), "hub/spawn-pending/erik") -@pytest.mark.parametrize("has_access", ["all", "user", "group", False]) +@pytest.mark.parametrize( + "has_access", ["all", "user", (pytest.param("group", id="in-group")), False] +) async def test_spawn_page_access( app, has_access, group, username, user, create_temp_role ): @@ -403,7 +407,9 @@ async def test_spawn_form(app): } -@pytest.mark.parametrize("has_access", ["all", "user", "group", False]) +@pytest.mark.parametrize( + "has_access", ["all", "user", (pytest.param("group", id="in-group")), False] +) async def test_spawn_form_other_user( app, username, user, group, create_temp_role, has_access ): @@ -624,7 +630,9 @@ async def test_user_redirect_hook(app, username): assert redirected_url.path == ujoin(app.base_url, 'user', username, 'terminals/1') -@pytest.mark.parametrize("has_access", ["all", "user", "group", False]) +@pytest.mark.parametrize( + "has_access", ["all", "user", (pytest.param("group", id="in-group")), False] +) async def test_other_user_url(app, username, user, group, create_temp_role, has_access): """Test accessing /user/someonelse/ URLs when the server is not running diff --git a/jupyterhub/tests/test_shares.py b/jupyterhub/tests/test_shares.py index db12757f..2d3d2c92 100644 --- a/jupyterhub/tests/test_shares.py +++ b/jupyterhub/tests/test_shares.py @@ -159,7 +159,9 @@ def expand_scopes(scope_str, user, group=None, share_with=None): return scopes -@pytest.mark.parametrize("share_with", ["user", "group"]) +@pytest.mark.parametrize( + "share_with", ["user", pytest.param("group", id="share_with=group")] +) def test_create_share(app, user, share_user, group, share_with): db = app.db spawner = user.spawner.orm_spawner @@ -427,7 +429,7 @@ def test_share_code_expires(app, user, share_user): "kind", [ ("user"), - ("group"), + (pytest.param("group", id="kind=group")), ], ) async def test_shares_api_user_group_doesnt_exist( From 48c046359f9ddc906c081a0795e26ef9b3deda6c Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 15 Apr 2024 10:06:04 +0200 Subject: [PATCH 43/43] remove unnecessary test parameter allow_all doesn't vary, so doesn't need to be a parameter --- jupyterhub/tests/test_auth.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index 7cfda23d..5ee5550c 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -609,22 +609,20 @@ async def test_auth_managed_groups( @pytest.mark.parametrize( - "allowed_users, allow_all, allow_existing_users", + "allowed_users, allow_existing_users", [ - ('specified', False, True), - ('', False, False), + ('specified', True), + ('', False), ], ) -async def test_allow_defaults( - app, user, allowed_users, allow_all, allow_existing_users -): +async def test_allow_defaults(app, user, allowed_users, allow_existing_users): if allowed_users: allowed_users = set(allowed_users.split(',')) else: allowed_users = set() authenticator = auth.Authenticator(allowed_users=allowed_users) authenticator.authenticate = lambda handler, data: data["username"] - assert authenticator.allow_all == allow_all + assert authenticator.allow_all is False assert authenticator.allow_existing_users == allow_existing_users # user was already in the database