From b7621ea82b7f1b0683f8943254c5b83190299da0 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Fri, 28 Mar 2025 08:55:41 -0700 Subject: [PATCH 1/4] Require different password for admins with dummyauthenticator Currently, admin users are even more insecure than otherwise with dummyauthenticator - anyone who knows the username of the admin can get in if they also know the password. This PR adds an additional layer of security - admins *must* login using a different, more secure (longer, per NIST guidelines) password. If they login using the regular password, no admin status for them. This mildly helpful in local testing and improves overall security posture. Where it really shines though, is in 'workshop' hubs. I've been running those for years now, both at UC Berkeley and now at 2i2c (with NASA Openscapes in particular). This was the usecase DummyAuth was written for :D It allows an instructor to share a single password with all the users in a secure way (they're all in a physical room, zoom, etc). The password is then changed after the workshop. However, admin access was not possible in this use case, as anyone guessing the admin's username can get in as admin. With this change, admin access is possible. --- jupyterhub/auth.py | 64 ++++++++++++++++++++++++++- jupyterhub/tests/test_dummyauth.py | 71 ++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index d5ba38dc..82e7b83c 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -20,7 +20,7 @@ except Exception as e: _pamela_error = e from tornado.concurrent import run_on_executor -from traitlets import Any, Bool, Dict, Integer, Set, Unicode, default, observe +from traitlets import Any, Bool, Dict, Integer, Set, Unicode, default, observe, validate from traitlets.config import LoggingConfigurable from .handlers.login import LoginHandler @@ -1510,6 +1510,63 @@ class DummyAuthenticator(Authenticator): """, ) + admin_password = Unicode( + None, + allow_none=True, + config=True, + help=""" + Set a global password that grants *admin* privileges to users logging in with this password. + + Must meet the following requirements: + - Be 32 characters or longer + - password for regular users must also be set + - Not be the same as the password + """, + ) + + @validate("admin_password") + def _validate_admin_password(self, proposal): + new = proposal.value + if new is None: + # Don't do anything if we're None + return None + if len(new) < 32: + raise ValueError( + f"{self.__class__.__name__}.admin_password must be at least 32 characters" + ) + if not self.password: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.password must be set if admin_password is set" + ) + if self.password == new: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.password and {self.__class__.__name__}.admin_password can not be the same" + ) + return new + + @validate("password") + def _validate_password(self, proposal): + new = proposal.value + if self.admin_password and not new: + # Checked here and in validating admin_password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.password must be set if admin_password is set" + ) + + if not new: + # If unset, let it be + return new + + if self.admin_password == new: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.password and {self.__class__.__name__}.admin_password can not be the same" + ) + + return new + def check_allow_config(self): super().check_allow_config() self.log.warning( @@ -1520,7 +1577,10 @@ class DummyAuthenticator(Authenticator): """Checks against a global password if it's been set. If not, allow any user/pass combo""" if self.password: if data['password'] == self.password: - return data['username'] + # Anyone logging in with the standard password is *never* admin + return {"name": data['username'], "admin": False} + if data['username'] in self.admin_users and data['password'] == self.admin_password: + return {"name": data["username"], "admin": True} return None return data['username'] diff --git a/jupyterhub/tests/test_dummyauth.py b/jupyterhub/tests/test_dummyauth.py index 12886c7b..6d2f47ec 100644 --- a/jupyterhub/tests/test_dummyauth.py +++ b/jupyterhub/tests/test_dummyauth.py @@ -2,6 +2,8 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import pytest + from jupyterhub.auth import DummyAuthenticator @@ -44,3 +46,72 @@ async def test_dummy_auth_with_global_password(): None, {'username': 'some_other_user', 'password': 'test_password'} ) assert authorized['name'] == 'some_other_user' + + +async def test_dummy_auth_admin_password_validation(): + authenticator = DummyAuthenticator() + + # Validate that password must also be set + with pytest.raises( + ValueError, + match="DummyAuthenticator.password must be set if admin_password is set", + ): + authenticator.admin_password = "a" * 32 + + # Validate length + authenticator.password = "password" + with pytest.raises( + ValueError, + match="DummyAuthenticator.admin_password must be at least 32 characters", + ): + authenticator.admin_password = "a" * 31 + + # Validate that the passwords aren't the same + authenticator.password = "a" * 32 + with pytest.raises( + ValueError, + match="DummyAuthenticator.password and DummyAuthenticator.admin_password can not be the same", + ): + authenticator.admin_password = "a" * 32 + + +async def test_dummy_auth_admin_password(): + authenticator = DummyAuthenticator() + authenticator.password = "pass" + authenticator.admin_password = "a" * 32 + + # Regular user, regular password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'pass'} + ) + assert authorized['name'] == 'test_user' + assert not authorized['admin'] + + # Regular user, admin password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'a' * 32} + ) + assert not authorized + + # Admin user, admin password + authenticator.admin_users = {"test_user"} + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'a' * 32} + ) + assert authorized['name'] == 'test_user' + assert authorized['admin'] + + # Admin user, regular password + authenticator.admin_users = {"test_user"} + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'pass'} + ) + assert authorized['name'] == 'test_user' + assert not authorized['admin'] + + # Regular user, wrong password + authenticator.admin_users = set() + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'blah'} + ) + assert not authorized From d45472a7fcff7e38f953ba4791b12ca1e57aeb16 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Mon, 31 Mar 2025 11:45:14 -0700 Subject: [PATCH 2/4] Partially move to another authenticator --- jupyterhub/auth.py | 62 +------------- jupyterhub/authenticators/__init__.py | 0 jupyterhub/authenticators/shared.py | 115 ++++++++++++++++++++++++++ pyproject.toml | 1 + 4 files changed, 117 insertions(+), 61 deletions(-) create mode 100644 jupyterhub/authenticators/__init__.py create mode 100644 jupyterhub/authenticators/shared.py diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 82e7b83c..1683e5a2 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -1510,63 +1510,6 @@ class DummyAuthenticator(Authenticator): """, ) - admin_password = Unicode( - None, - allow_none=True, - config=True, - help=""" - Set a global password that grants *admin* privileges to users logging in with this password. - - Must meet the following requirements: - - Be 32 characters or longer - - password for regular users must also be set - - Not be the same as the password - """, - ) - - @validate("admin_password") - def _validate_admin_password(self, proposal): - new = proposal.value - if new is None: - # Don't do anything if we're None - return None - if len(new) < 32: - raise ValueError( - f"{self.__class__.__name__}.admin_password must be at least 32 characters" - ) - if not self.password: - # Checked here and in validating password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.password must be set if admin_password is set" - ) - if self.password == new: - # Checked here and in validating password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.password and {self.__class__.__name__}.admin_password can not be the same" - ) - return new - - @validate("password") - def _validate_password(self, proposal): - new = proposal.value - if self.admin_password and not new: - # Checked here and in validating admin_password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.password must be set if admin_password is set" - ) - - if not new: - # If unset, let it be - return new - - if self.admin_password == new: - # Checked here and in validating password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.password and {self.__class__.__name__}.admin_password can not be the same" - ) - - return new - def check_allow_config(self): super().check_allow_config() self.log.warning( @@ -1577,10 +1520,7 @@ class DummyAuthenticator(Authenticator): """Checks against a global password if it's been set. If not, allow any user/pass combo""" if self.password: if data['password'] == self.password: - # Anyone logging in with the standard password is *never* admin - return {"name": data['username'], "admin": False} - if data['username'] in self.admin_users and data['password'] == self.admin_password: - return {"name": data["username"], "admin": True} + return data["username"] return None return data['username'] diff --git a/jupyterhub/authenticators/__init__.py b/jupyterhub/authenticators/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/jupyterhub/authenticators/shared.py b/jupyterhub/authenticators/shared.py new file mode 100644 index 00000000..e3b78da9 --- /dev/null +++ b/jupyterhub/authenticators/shared.py @@ -0,0 +1,115 @@ +from ..auth import Authenticator +from traitlets import TraitError, default, Unicode, validate + +class SharedPasswordAuthenticator(Authenticator): + """ + Authenticator with static shared passwords. + """ + @default("allow_all") + def _allow_all_default(self): + if self.allowed_users: + return False + else: + # allow all by default + return True + + global_password = Unicode( + None, + allow_none=True, # Allow None here, so we can provide a better error message in our validation + config=True, + help=""" + Set a global password for all *non admin* users wanting to log in. + + Must be 8 characters or longer. + + If `allowed_users` is not set, any user with any username can login with this password. + If `allowed_users` is set, only the set of usernames present in `allowed_users` can log in- + although they all use the same password. + + If `admin_users` is set, those users *must* use `admin_password` to log in. + """, + ) + + admin_password = Unicode( + None, + allow_none=True, + config=True, + help=""" + Set a global password that grants *admin* privileges to users logging in with this password. + + Must meet the following requirements: + - Be 32 characters or longer + - password for regular users (global_password) must also be set + - Not be the same as the password + + Admin access to the hub is disabled if this is not set. + """, + ) + + @validate("admin_password") + def _validate_admin_password(self, proposal): + new = proposal.value + if new is None: + # Don't do anything if we're None + return None + if len(new) < 32: + raise ValueError( + f"{self.__class__.__name__}.admin_password must be at least 32 characters" + ) + if not self.global_password: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.global_password must be set if admin_password is set" + ) + if self.global_password == new: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.global_password and {self.__class__.__name__}.admin_password can not be the same" + ) + return new + + @validate("password") + def _validate_password(self, proposal): + new = proposal.value + + if new is None: + raise ValueError( + f"{self.__class__.__name__}.global_password must be set to use {self.__class__.__name__}" + ) + if new and len(new) < 8: + raise ValueError( + f"{self.__class__.__name__}.global_password must be at least 8 characters long" + ) + + if self.admin_password and not new: + # Checked here and in validating admin_password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.global_password must be set if admin_password is set" + ) + + if not new: + # If unset, let it be + return new + + if self.admin_password == new: + # Checked here and in validating password, to ensure we don't miss issues due to ordering + raise ValueError( + f"{self.__class__.__name__}.global_password and {self.__class__.__name__}.admin_password can not be the same" + ) + + return new + + + async def authenticate(self, handler, data): + """Checks against a global password if it's been set. If not, allow any user/pass combo""" + if data['username'] in self.admin_users: + # Admin user + if data['password'] == self.admin_password: + return {"name": data["username"], "admin": True} + else: + # Regular user + if data['password'] == self.global_password: + # Anyone logging in with the standard password is *never* admin + return {"name": data['username'], "admin": False} + + return None \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 64091de6..a1256e11 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,7 @@ jupyterhub-singleuser = "jupyterhub.singleuser:main" default = "jupyterhub.auth:PAMAuthenticator" pam = "jupyterhub.auth:PAMAuthenticator" dummy = "jupyterhub.auth:DummyAuthenticator" +sharedpassword = "jupyterhub.authenticators.shared:SharedPasswordAuthenticator" null = "jupyterhub.auth:NullAuthenticator" [project.entry-points."jupyterhub.proxies"] From 2457813432c068b9079cee094de8edcedb050dfa Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 31 Mar 2025 18:50:16 +0000 Subject: [PATCH 3/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- jupyterhub/auth.py | 2 +- jupyterhub/authenticators/shared.py | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 1683e5a2..0f4e773a 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -20,7 +20,7 @@ except Exception as e: _pamela_error = e from tornado.concurrent import run_on_executor -from traitlets import Any, Bool, Dict, Integer, Set, Unicode, default, observe, validate +from traitlets import Any, Bool, Dict, Integer, Set, Unicode, default, observe from traitlets.config import LoggingConfigurable from .handlers.login import LoginHandler diff --git a/jupyterhub/authenticators/shared.py b/jupyterhub/authenticators/shared.py index e3b78da9..7a570cf4 100644 --- a/jupyterhub/authenticators/shared.py +++ b/jupyterhub/authenticators/shared.py @@ -1,10 +1,13 @@ +from traitlets import Unicode, default, validate + from ..auth import Authenticator -from traitlets import TraitError, default, Unicode, validate + class SharedPasswordAuthenticator(Authenticator): """ Authenticator with static shared passwords. """ + @default("allow_all") def _allow_all_default(self): if self.allowed_users: @@ -15,7 +18,7 @@ class SharedPasswordAuthenticator(Authenticator): global_password = Unicode( None, - allow_none=True, # Allow None here, so we can provide a better error message in our validation + allow_none=True, # Allow None here, so we can provide a better error message in our validation config=True, help=""" Set a global password for all *non admin* users wanting to log in. @@ -99,7 +102,6 @@ class SharedPasswordAuthenticator(Authenticator): return new - async def authenticate(self, handler, data): """Checks against a global password if it's been set. If not, allow any user/pass combo""" if data['username'] in self.admin_users: @@ -112,4 +114,4 @@ class SharedPasswordAuthenticator(Authenticator): # Anyone logging in with the standard password is *never* admin return {"name": data['username'], "admin": False} - return None \ No newline at end of file + return None From 7522d2c73a984e91bebea573de86171a6d67297a Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 2 Apr 2025 12:16:22 +0200 Subject: [PATCH 4/4] flesh out SharedPasswordAuthenticator - add docs, tests - deprecate DummyAuthenticator.password, pointing to new class - accept no password as valid config (no login possible) - log warnings for suspicious config (e.g. passwords not set, admin password set, but no admin users, etc.) --- docs/source/reference/api/auth.md | 23 +++- docs/source/reference/authenticators.md | 42 +++++- jupyterhub/auth.py | 13 +- jupyterhub/authenticators/shared.py | 148 +++++++++++++-------- jupyterhub/tests/test_dummyauth.py | 70 ---------- jupyterhub/tests/test_shared_password.py | 162 +++++++++++++++++++++++ pyproject.toml | 2 +- 7 files changed, 317 insertions(+), 143 deletions(-) create mode 100644 jupyterhub/tests/test_shared_password.py diff --git a/docs/source/reference/api/auth.md b/docs/source/reference/api/auth.md index 7f4661b4..094d5953 100644 --- a/docs/source/reference/api/auth.md +++ b/docs/source/reference/api/auth.md @@ -1,33 +1,42 @@ # Authenticators -## Module: {mod}`jupyterhub.auth` - ```{eval-rst} -.. automodule:: jupyterhub.auth +.. module:: jupyterhub.auth ``` -### {class}`Authenticator` +## {class}`Authenticator` ```{eval-rst} .. autoconfigurable:: Authenticator :members: ``` -### {class}`LocalAuthenticator` +## {class}`LocalAuthenticator` ```{eval-rst} .. autoconfigurable:: LocalAuthenticator :members: ``` -### {class}`PAMAuthenticator` +## {class}`PAMAuthenticator` ```{eval-rst} .. autoconfigurable:: PAMAuthenticator ``` -### {class}`DummyAuthenticator` +## {class}`DummyAuthenticator` ```{eval-rst} .. autoconfigurable:: DummyAuthenticator ``` + +```{eval-rst} +.. module:: jupyterhub.authenticators.shared +``` + +## {class}`SharedPasswordAuthenticator` + +```{eval-rst} +.. autoconfigurable:: SharedPasswordAuthenticator + :no-inherited-members: +``` diff --git a/docs/source/reference/authenticators.md b/docs/source/reference/authenticators.md index eb1d95ec..4131c704 100644 --- a/docs/source/reference/authenticators.md +++ b/docs/source/reference/authenticators.md @@ -36,16 +36,50 @@ 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 a global password has been set. Once set, any username will -still be accepted but the correct password will need to be provided. +When testing, it may be helpful to use the {class}`~.jupyterhub.auth.DummyAuthenticator`: + +```python +c.JupyterHub.authenticator_class = "dummy" +# always a good idea to limit to localhost when testing with an insecure config +c.JupyterHub.ip = "127.0.0.1" +``` + +This allows for any username and password to login, and is _wildly_ insecure. :::{versionadded} 5.0 The DummyAuthenticator's default `allow_all` is True, unlike most other Authenticators. ::: +:::{deprecated} 5.3 +Setting a password on DummyAuthenticator is deprecated. +Use the new {class}`~.jupyterhub.authenticators.shared.SharedPasswordAuthenticator` +if you want to set a shared password for users. +::: + +## Shared Password Authenticator + +:::{versionadded} 5.3 +{class}`~.jupyterhub.authenticators.shared.SharedPasswordAuthenticator` is added and [DummyAuthenticator.password](#DummyAuthenticator.password) is deprecated. +::: + +For short-term deployments like workshops where there is no real user data to protect and you trust users to not abuse the system or each other, +{class}`~.jupyterhub.authenticators.shared.SharedPasswordAuthenticator` can be used. + +Set a [user password](#SharedPasswordAuthenticator.user_password) for users to login: + +```python +c.JupyterHub.authenticator_class = "shared-password" +c.SharedPasswordAuthenticator.user_password = "my-workshop-2042" +``` + +You can also grant admin users access by adding them to `admin_users` and setting a separate [admin password](#SharedPasswordAuthenticator.admin_password): + +```python +c.Authenticator.admin_users = {"danger", "eggs"} +c.SharedPasswordAuthenticator.admin_password = "extra-super-secret-secure-password" +``` + ## Additional Authenticators Additional authenticators can be found on GitHub diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 0f4e773a..fab1cf43 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -1504,12 +1504,19 @@ class DummyAuthenticator(Authenticator): password = Unicode( config=True, help=""" - Set a global password for all users wanting to log in. - - This allows users with any username to log in with the same static password. + .. deprecated:: 5.3 + + Setting a password in DummyAuthenticator is deprecated. + Use `SharedPasswordAuthenticator` instead. """, ) + @observe("password") + def _password_changed(self, change): + msg = "DummyAuthenticator.password is deprecated in JupyterHub 5.3. Use SharedPasswordAuthenticator.user_password instead." + warnings.warn(msg, DeprecationWarning) + self.log.warning(msg) + def check_allow_config(self): super().check_allow_config() self.log.warning( diff --git a/jupyterhub/authenticators/shared.py b/jupyterhub/authenticators/shared.py index 7a570cf4..820fadfd 100644 --- a/jupyterhub/authenticators/shared.py +++ b/jupyterhub/authenticators/shared.py @@ -1,4 +1,6 @@ -from traitlets import Unicode, default, validate +from secrets import compare_digest + +from traitlets import Unicode, validate from ..auth import Authenticator @@ -6,28 +8,40 @@ from ..auth import Authenticator class SharedPasswordAuthenticator(Authenticator): """ Authenticator with static shared passwords. + + For use in short-term deployments with negligible security concerns. + + Enable with:: + + c.JupyterHub.authenticator_class = "shared-password" + + .. warning:: + This is an insecure Authenticator only appropriate for short-term + deployments with no requirement to protect users from each other. + + - The password is stored in plain text at rest in config + - Anyone with the password can login as **any user** + - All users are able to login as all other (non-admin) users with the same password """ - @default("allow_all") - def _allow_all_default(self): - if self.allowed_users: - return False - else: - # allow all by default - return True + _USER_PASSWORD_MIN_LENGTH = 8 + _ADMIN_PASSWORD_MIN_LENGTH = 32 - global_password = Unicode( + user_password = Unicode( None, - allow_none=True, # Allow None here, so we can provide a better error message in our validation + allow_none=True, config=True, - help=""" + help=f""" Set a global password for all *non admin* users wanting to log in. - Must be 8 characters or longer. + Must be {_USER_PASSWORD_MIN_LENGTH} characters or longer. - If `allowed_users` is not set, any user with any username can login with this password. - If `allowed_users` is set, only the set of usernames present in `allowed_users` can log in- - although they all use the same password. + If not set, regular users cannot login. + + If `allow_all` is True, anybody can register unlimited new users with any username by logging in with this password. + Users may be allowed by name by specifying `allowed_users`. + + Any user will also be able to login as **any other non-admin user** with this password. If `admin_users` is set, those users *must* use `admin_password` to log in. """, @@ -37,81 +51,99 @@ class SharedPasswordAuthenticator(Authenticator): None, allow_none=True, config=True, - help=""" + help=f""" Set a global password that grants *admin* privileges to users logging in with this password. + Only usernames declared in `admin_users` may login with this password. Must meet the following requirements: - - Be 32 characters or longer - - password for regular users (global_password) must also be set - - Not be the same as the password - Admin access to the hub is disabled if this is not set. + - Be {_ADMIN_PASSWORD_MIN_LENGTH} characters or longer + - Not be the same as `user_password` + + If not set, admin users cannot login. """, ) @validate("admin_password") def _validate_admin_password(self, proposal): new = proposal.value - if new is None: - # Don't do anything if we're None + trait_name = f"{self.__class__.__name__}.{proposal.trait.name}" + + if not new: + # no admin password; do nothing return None - if len(new) < 32: + if len(new) < self._ADMIN_PASSWORD_MIN_LENGTH: raise ValueError( - f"{self.__class__.__name__}.admin_password must be at least 32 characters" + f"{trait_name} must be at least {self._ADMIN_PASSWORD_MIN_LENGTH} characters, not {len(new)}." ) - if not self.global_password: + if self.user_password == new: # Checked here and in validating password, to ensure we don't miss issues due to ordering raise ValueError( - f"{self.__class__.__name__}.global_password must be set if admin_password is set" - ) - if self.global_password == new: - # Checked here and in validating password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.global_password and {self.__class__.__name__}.admin_password can not be the same" + f"{self.__class__.__name__}.user_password and {trait_name} cannot be the same" ) return new - @validate("password") + @validate("user_password") def _validate_password(self, proposal): new = proposal.value - - if new is None: - raise ValueError( - f"{self.__class__.__name__}.global_password must be set to use {self.__class__.__name__}" - ) - if new and len(new) < 8: - raise ValueError( - f"{self.__class__.__name__}.global_password must be at least 8 characters long" - ) - - if self.admin_password and not new: - # Checked here and in validating admin_password, to ensure we don't miss issues due to ordering - raise ValueError( - f"{self.__class__.__name__}.global_password must be set if admin_password is set" - ) + trait_name = f"{self.__class__.__name__}.{proposal.trait.name}" if not new: - # If unset, let it be - return new - + # no user password; do nothing + return None + if len(new) < self._USER_PASSWORD_MIN_LENGTH: + raise ValueError( + f"{trait_name} must be at least {self._USER_PASSWORD_MIN_LENGTH} characters long, got {len(new)} characters" + ) if self.admin_password == new: # Checked here and in validating password, to ensure we don't miss issues due to ordering raise ValueError( - f"{self.__class__.__name__}.global_password and {self.__class__.__name__}.admin_password can not be the same" + f"{trait_name} and {self.__class__.__name__}.admin_password cannot be the same" ) - return new + def check_allow_config(self): + """Validate and warn about any suspicious allow config""" + super().check_allow_config() + clsname = self.__class__.__name__ + if self.admin_password and not self.admin_users: + self.log.warning( + f"{clsname}.admin_password set, but {clsname}.admin_users is not." + " No admin users will be able to login." + f" Add usernames to {clsname}.admin_users to grant users admin permissions." + ) + if self.admin_users and not self.admin_password: + self.log.warning( + f"{clsname}.admin_users set, but {clsname}.admin_password is not." + " No admin users will be able to login." + f" Set {clsname}.admin_password to allow admins to login." + ) + if not self.user_password: + if not self.admin_password: + # log as an error, but don't raise, because disabling all login is valid + self.log.error( + f"Neither {clsname}.admin_password nor {clsname}.user_password is set." + " Nobody will be able to login!" + ) + else: + self.log.warning( + f"{clsname}.user_password not set." + " No non-admin users will be able to login." + ) + async def authenticate(self, handler, data): - """Checks against a global password if it's been set. If not, allow any user/pass combo""" - if data['username'] in self.admin_users: + """Checks against shared password""" + if data["username"] in self.admin_users: # Admin user - if data['password'] == self.admin_password: + if self.admin_password and compare_digest( + data["password"], self.admin_password + ): return {"name": data["username"], "admin": True} else: - # Regular user - if data['password'] == self.global_password: + if self.user_password and compare_digest( + data["password"], self.user_password + ): # Anyone logging in with the standard password is *never* admin - return {"name": data['username'], "admin": False} + return {"name": data["username"], "admin": False} return None diff --git a/jupyterhub/tests/test_dummyauth.py b/jupyterhub/tests/test_dummyauth.py index 6d2f47ec..a88aab56 100644 --- a/jupyterhub/tests/test_dummyauth.py +++ b/jupyterhub/tests/test_dummyauth.py @@ -2,7 +2,6 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. -import pytest from jupyterhub.auth import DummyAuthenticator @@ -46,72 +45,3 @@ async def test_dummy_auth_with_global_password(): None, {'username': 'some_other_user', 'password': 'test_password'} ) assert authorized['name'] == 'some_other_user' - - -async def test_dummy_auth_admin_password_validation(): - authenticator = DummyAuthenticator() - - # Validate that password must also be set - with pytest.raises( - ValueError, - match="DummyAuthenticator.password must be set if admin_password is set", - ): - authenticator.admin_password = "a" * 32 - - # Validate length - authenticator.password = "password" - with pytest.raises( - ValueError, - match="DummyAuthenticator.admin_password must be at least 32 characters", - ): - authenticator.admin_password = "a" * 31 - - # Validate that the passwords aren't the same - authenticator.password = "a" * 32 - with pytest.raises( - ValueError, - match="DummyAuthenticator.password and DummyAuthenticator.admin_password can not be the same", - ): - authenticator.admin_password = "a" * 32 - - -async def test_dummy_auth_admin_password(): - authenticator = DummyAuthenticator() - authenticator.password = "pass" - authenticator.admin_password = "a" * 32 - - # Regular user, regular password - authorized = await authenticator.get_authenticated_user( - None, {'username': 'test_user', 'password': 'pass'} - ) - assert authorized['name'] == 'test_user' - assert not authorized['admin'] - - # Regular user, admin password - authorized = await authenticator.get_authenticated_user( - None, {'username': 'test_user', 'password': 'a' * 32} - ) - assert not authorized - - # Admin user, admin password - authenticator.admin_users = {"test_user"} - authorized = await authenticator.get_authenticated_user( - None, {'username': 'test_user', 'password': 'a' * 32} - ) - assert authorized['name'] == 'test_user' - assert authorized['admin'] - - # Admin user, regular password - authenticator.admin_users = {"test_user"} - authorized = await authenticator.get_authenticated_user( - None, {'username': 'test_user', 'password': 'pass'} - ) - assert authorized['name'] == 'test_user' - assert not authorized['admin'] - - # Regular user, wrong password - authenticator.admin_users = set() - authorized = await authenticator.get_authenticated_user( - None, {'username': 'test_user', 'password': 'blah'} - ) - assert not authorized diff --git a/jupyterhub/tests/test_shared_password.py b/jupyterhub/tests/test_shared_password.py new file mode 100644 index 00000000..38e06b35 --- /dev/null +++ b/jupyterhub/tests/test_shared_password.py @@ -0,0 +1,162 @@ +import pytest +from traitlets.config import Config + +from jupyterhub.authenticators.shared import SharedPasswordAuthenticator + + +@pytest.fixture +def admin_password(): + return "a" * 32 + + +@pytest.fixture +def user_password(): + return "user_password" + + +@pytest.fixture +def authenticator(admin_password, user_password): + return SharedPasswordAuthenticator( + admin_password=admin_password, + user_password=user_password, + admin_users={"admin"}, + allow_all=True, + ) + + +async def test_password_validation(): + authenticator = SharedPasswordAuthenticator() + # Validate length + with pytest.raises( + ValueError, + match="admin_password must be at least 32 characters", + ): + authenticator.admin_password = "a" * 31 + + with pytest.raises( + ValueError, + match="user_password must be at least 8 characters", + ): + authenticator.user_password = "a" * 7 + + # Validate that the passwords aren't the same + authenticator.user_password = "a" * 32 + with pytest.raises( + ValueError, + match="SharedPasswordAuthenticator.user_password and SharedPasswordAuthenticator.admin_password cannot be the same", + ): + authenticator.admin_password = "a" * 32 + + # ok + authenticator.admin_password = "a" * 33 + + # check collision in the other order + with pytest.raises( + ValueError, + match="SharedPasswordAuthenticator.user_password and SharedPasswordAuthenticator.admin_password cannot be the same", + ): + authenticator.user_password = "a" * 33 + + +async def test_admin_password(authenticator, user_password, admin_password): + # Regular user, regular password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': user_password} + ) + assert authorized['name'] == 'test_user' + assert not authorized['admin'] + + # Regular user, admin password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': admin_password} + ) + assert not authorized + + # Admin user, admin password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'admin', 'password': admin_password} + ) + assert authorized['name'] == 'admin' + assert authorized['admin'] + + # Admin user, regular password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'admin', 'password': user_password} + ) + assert not authorized + + # Regular user, wrong password + authorized = await authenticator.get_authenticated_user( + None, {'username': 'test_user', 'password': 'blah'} + ) + assert not authorized + + # New username, allow_all is False + authenticator.allow_all = False + authorized = await authenticator.get_authenticated_user( + None, {'username': 'new_user', 'password': 'user_password'} + ) + assert not authorized + + +async def test_empty_passwords(): + authenticator = SharedPasswordAuthenticator( + allow_all=True, + admin_users={"admin"}, + user_password="", + admin_password="", + ) + authorized = await authenticator.get_authenticated_user( + None, {'username': 'admin', 'password': ''} + ) + assert not authorized + authorized = await authenticator.get_authenticated_user( + None, {'username': 'user', 'password': ''} + ) + assert not authorized + + +@pytest.mark.parametrize( + "auth_config, warns, not_warns", + [ + pytest.param({}, "nobody can login", "", id="default"), + pytest.param( + {"allow_all": True}, + "Nobody will be able to login", + "regular users", + id="no passwords", + ), + pytest.param( + {"admin_password": "a" * 32}, "admin_users is not", "", id="no admin_users" + ), + pytest.param( + {"admin_users": {"admin"}}, + "admin_password is not", + "", + id="no admin_password", + ), + pytest.param( + {"admin_users": {"admin"}, "admin_password": "a" * 32, "allow_all": True}, + "No non-admin users will be able to login", + "", + id="only_admin", + ), + ], +) +def test_check_allow_config(caplog, auth_config, warns, not_warns): + # check log warnings + config = Config() + for key, value in auth_config.items(): + setattr(config.SharedPasswordAuthenticator, key, value) + authenticator = SharedPasswordAuthenticator(config=config) + authenticator.check_allow_config() + if warns: + if isinstance(warns, str): + warns = [warns] + for snippet in warns: + assert snippet in caplog.text + if not_warns: + if isinstance(not_warns, str): + not_warns = [not_warns] + for snippet in not_warns: + assert snippet not in caplog.text diff --git a/pyproject.toml b/pyproject.toml index a1256e11..e4aa5e24 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,7 +67,7 @@ jupyterhub-singleuser = "jupyterhub.singleuser:main" default = "jupyterhub.auth:PAMAuthenticator" pam = "jupyterhub.auth:PAMAuthenticator" dummy = "jupyterhub.auth:DummyAuthenticator" -sharedpassword = "jupyterhub.authenticators.shared:SharedPasswordAuthenticator" +shared-password = "jupyterhub.authenticators.shared:SharedPasswordAuthenticator" null = "jupyterhub.auth:NullAuthenticator" [project.entry-points."jupyterhub.proxies"]