From 1f2d56e60515684aa6893919065e80993436345d Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Fri, 11 Aug 2023 16:31:47 +0300 Subject: [PATCH] make auth.salt parameter optional Used implementation of the hasher includes salt itself, thus additional salt is optional and can be safely (in terms of security) treat as empty string --- docs/architecture.rst | 2 +- docs/configuration.rst | 2 +- docs/faq.rst | 4 +++- src/ahriman/application/handlers/users.py | 2 +- src/ahriman/core/auth/mapping.py | 2 +- src/ahriman/core/configuration/schema.py | 7 +++++-- tests/ahriman/application/handlers/test_handler_users.py | 9 ++++++--- 7 files changed, 18 insertions(+), 10 deletions(-) diff --git a/docs/architecture.rst b/docs/architecture.rst index 3e1e037c..8621e073 100644 --- a/docs/architecture.rst +++ b/docs/architecture.rst @@ -231,7 +231,7 @@ The package provides several authorization methods: disabled, based on configura Disabled (default) authorization provider just allows everything for everyone and does not have any specific configuration (it uses some default configuration parameters though). It also provides generic interface for derived classes. -Mapping (aka configuration) provider uses hashed passwords with salt from the database in order to authenticate users. This provider also enables user permission checking (read/write) (authorization). Thus, it defines the following methods: +Mapping (aka configuration) provider uses hashed passwords with optional salt from the database in order to authenticate users. This provider also enables user permission checking (read/write) (authorization). Thus, it defines the following methods: * ``check_credentials`` - user password validation (authentication). * ``verify_access`` - user permission validation (authorization). diff --git a/docs/configuration.rst b/docs/configuration.rst index baa85dec..b49057b5 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -54,7 +54,7 @@ Base authorization settings. ``OAuth`` provider requires ``aioauth-client`` libr * ``max_age`` - parameter which controls both cookie expiration and token expiration inside the service, integer, optional, default is 7 days. * ``oauth_provider`` - OAuth2 provider class name as is in ``aioauth-client`` (e.g. ``GoogleClient``, ``GithubClient`` etc), string, required in case if ``oauth`` is used. * ``oauth_scopes`` - scopes list for OAuth2 provider, which will allow retrieving user email (which is used for checking user permissions), e.g. ``https://www.googleapis.com/auth/userinfo.email`` for ``GoogleClient`` or ``user:email`` for ``GithubClient``, space separated list of strings, required in case if ``oauth`` is used. -* ``salt`` - password hash salt, string, required in case if authorization enabled (automatically generated by ``user-add`` subcommand). +* ``salt`` - additional password hash salt, string, optional. Authorized users are stored inside internal database, if any of external provides are used the password field for non-service users must be empty. diff --git a/docs/faq.rst b/docs/faq.rst index a09f079e..a52ce94a 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -870,6 +870,8 @@ How to enable basic authorization target = configuration salt = somerandomstring + The ``salt`` parameter is optional, but recommended. + #. In order to provide access for reporting from application instances you can (recommended way) use unix sockets by configuring the following (note, that it requires ``python-requests-unixsocket`` package to be installed): @@ -934,7 +936,7 @@ How to enable OAuth authorization Configure ``oauth_provider`` and ``oauth_scopes`` in case if you would like to use different from Google provider. Scope must grant access to user email. ``web.address`` is required to make callback URL available from internet. #. - If you are not going to use unix socket, you also need to create service user (remember to set ``auth.salt`` option before): + If you are not going to use unix socket, you also need to create service user (remember to set ``auth.salt`` option before if required): .. code-block:: shell diff --git a/src/ahriman/application/handlers/users.py b/src/ahriman/application/handlers/users.py index b7ee8bea..1c133965 100644 --- a/src/ahriman/application/handlers/users.py +++ b/src/ahriman/application/handlers/users.py @@ -52,7 +52,7 @@ def run(cls, args: argparse.Namespace, architecture: str, configuration: Configu if args.action == Action.Update: user = Users.user_create(args) # if password is left blank we are not going to require salt to be set - salt = configuration.get("auth", "salt") if user.password else "" + salt = configuration.get("auth", "salt", fallback="") if user.password else "" database.user_update(user.hash_password(salt)) elif args.action == Action.List: users = database.user_list(args.username, args.role) diff --git a/src/ahriman/core/auth/mapping.py b/src/ahriman/core/auth/mapping.py index 00991230..3217b619 100644 --- a/src/ahriman/core/auth/mapping.py +++ b/src/ahriman/core/auth/mapping.py @@ -46,7 +46,7 @@ def __init__(self, configuration: Configuration, database: SQLite, """ Auth.__init__(self, configuration, provider) self.database = database - self.salt = configuration.get("auth", "salt") + self.salt = configuration.get("auth", "salt", fallback="") async def check_credentials(self, username: str | None, password: str | None) -> bool: """ diff --git a/src/ahriman/core/configuration/schema.py b/src/ahriman/core/configuration/schema.py index aea82ae1..c891bc80 100644 --- a/src/ahriman/core/configuration/schema.py +++ b/src/ahriman/core/configuration/schema.py @@ -93,9 +93,12 @@ "type": "string", "oneof": [ {"allowed": ["disabled"]}, - {"allowed": ["configuration", "mapping"], "dependencies": ["salt"]}, + {"allowed": ["configuration", "mapping"]}, {"allowed": ["oauth"], "dependencies": [ - "client_id", "client_secret", "oauth_provider", "oauth_scopes", "salt" + "client_id", + "client_secret", + "oauth_provider", + "oauth_scopes", ]}, ], }, diff --git a/tests/ahriman/application/handlers/test_handler_users.py b/tests/ahriman/application/handlers/test_handler_users.py index dd008301..2c050c0c 100644 --- a/tests/ahriman/application/handlers/test_handler_users.py +++ b/tests/ahriman/application/handlers/test_handler_users.py @@ -54,16 +54,19 @@ def test_run(args: argparse.Namespace, configuration: Configuration, database: S def test_run_empty_salt(args: argparse.Namespace, configuration: Configuration, mocker: MockerFixture) -> None: """ - must raise exception if salt is required, but not set + must process users with empty password salt """ configuration.remove_option("auth", "salt") args = _default_args(args) user = User(username=args.username, password=args.password, access=args.role, packager_id=args.packager, key=args.key) mocker.patch("ahriman.models.user.User.hash_password", return_value=user) + create_user_mock = mocker.patch("ahriman.application.handlers.Users.user_create", return_value=user) + update_mock = mocker.patch("ahriman.core.database.SQLite.user_update") - with pytest.raises(configparser.NoOptionError): - Users.run(args, "x86_64", configuration, report=False) + Users.run(args, "x86_64", configuration, report=False) + create_user_mock.assert_called_once_with(args) + update_mock.assert_called_once_with(user) def test_run_empty_salt_without_password(args: argparse.Namespace, configuration: Configuration, database: SQLite,