review unsafe commands access

Some commands were made unsafe in old versions, but nowadays they can be
run without having special privileges.

There was also a bug in which status commands were not available if you
are not ahriman user and unix socket is used. It has been fixed by
switching to manual socket creation (see also
aio-libs/aiohttp#4155)

docs/configuration.rst

@@ -242,4 +242,5 @@ Web server settings. If any of ``host``/``port`` is not set, web integration wil
 * ``static_path`` - path to directory with static files, string, required.
 * ``templates`` - path to templates directory, string, required.
 * ``unix_socket`` - path to the listening unix socket, string, optional. If set, server will create the socket on the specified address which can (and will) be used by application. Note, that unlike usual host/port configuration, unix socket allows to perform requests without authorization.
+* ``unix_socket_unsafe`` - set unsafe (o+w) permissions to unix socket, boolean, optional, default ``yes``. This option is enabled by default, because it is supposed that unix socket is created in safe environment (only web service is supposed to be used in unsafe), but it can be disabled by configuration.
 * ``username`` - username to authorize in web service in order to update service status, string, required in case if authorization enabled.  

package/share/ahriman/settings/ahriman.ini

@@ -65,4 +65,5 @@ debug_check_host = no
 debug_allowed_hosts =
 host = 127.0.0.1
 static_path = /usr/share/ahriman/templates/static
-templates = /usr/share/ahriman/templates
+templates = /usr/share/ahriman/templates
+unix_socket_unsafe = yes

src/ahriman/application/ahriman.py

@@ -399,7 +399,8 @@ def _set_patch_list_parser(root: SubParserAction) -> argparse.ArgumentParser:
     parser.add_argument("-e", "--exit-code", help="return non-zero exit status if result is empty", action="store_true")
     parser.add_argument("-v", "--variable", help="if set, show only patches for specified PKGBUILD variables",
                         action="append")
-    parser.set_defaults(handler=handlers.Patch, action=Action.List, architecture=[""], lock=None, report=False)
+    parser.set_defaults(handler=handlers.Patch, action=Action.List, architecture=[""], lock=None, report=False,
+                        unsafe=True)
     return parser
 
 
@@ -718,7 +719,7 @@ def _set_repo_tree_parser(root: SubParserAction) -> argparse.ArgumentParser:
     parser = root.add_parser("repo-tree", help="dump repository tree",
                              description="dump repository tree based on packages dependencies",
                              formatter_class=_formatter)
-    parser.set_defaults(handler=handlers.Structure, lock=None, report=False, quiet=True)
+    parser.set_defaults(handler=handlers.Structure, lock=None, report=False, quiet=True, unsafe=True)
     return parser
 
 
@@ -814,7 +815,7 @@ def _set_user_add_parser(root: SubParserAction) -> argparse.ArgumentParser:
                         type=UserAccess, choices=enum_values(UserAccess), default=UserAccess.Read)
     parser.add_argument("-s", "--secure", help="set file permissions to user-only", action="store_true")
     parser.set_defaults(handler=handlers.Users, action=Action.Update, architecture=[""], lock=None, report=False,
-                        quiet=True, unsafe=True)
+                        quiet=True)
     return parser
 
 
@@ -854,7 +855,7 @@ def _set_user_remove_parser(root: SubParserAction) -> argparse.ArgumentParser:
                              formatter_class=_formatter)
     parser.add_argument("username", help="username for web service")
     parser.set_defaults(handler=handlers.Users, action=Action.Remove, architecture=[""], lock=None, report=False,  # nosec
-                        password="", quiet=True, unsafe=True)
+                        password="", quiet=True)
     return parser
 
 

src/ahriman/core/status/web_client.py

@@ -160,7 +160,9 @@ def _package_url(self, package_base: str = "") -> str:
         Returns:
             str: full url of web service for specific package base
         """
-        return f"{self.address}/api/v1/packages/{package_base}"
+        # in case if unix socket is used we need to normalize url
+        suffix = f"/{package_base}" if package_base else ""
+        return f"{self.address}/api/v1/packages{suffix}"
 
     def add(self, package: Package, status: BuildStatusEnum) -> None:
         """

src/ahriman/web/web.py

@@ -20,8 +20,10 @@
 import aiohttp_jinja2
 import jinja2
 import logging
+import socket
 
 from aiohttp import web
+from typing import Optional
 
 from ahriman.core.auth import Auth
 from ahriman.core.configuration import Configuration
@@ -37,6 +39,39 @@
 __all__ = ["on_shutdown", "on_startup", "run_server", "setup_service"]
 
 
+def create_socket(configuration: Configuration, application: web.Application) -> Optional[socket.socket]:
+    """
+    create unix socket based on configuration option
+
+    Args:
+        configuration(Configuration): configuration instance
+        application(web.Application): web application instance
+
+    Returns:
+        Optional[socket.socket]: unix socket object if set by option
+    """
+    unix_socket = configuration.getpath("web", "unix_socket", fallback=None)
+    if unix_socket is None:
+        return None  # no option set
+    # create unix socket and bind it
+    unix_socket.unlink(missing_ok=True)  # remove socket file if it wasn't removed somehow before
+    sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+    sock.bind(str(unix_socket))
+    # allow everyone to write to the socket, otherwise API methods are not allowed by non-ahriman user
+    # by default sockets are created with same rights as directory is, but it seems that x bit is not really required
+    # see also https://github.com/aio-libs/aiohttp/issues/4155
+    if configuration.getboolean("web", "unix_socket_unsafe", fallback=True):
+        unix_socket.chmod(0o666)  # for the glory of satan of course
+
+    # register socket removal
+    async def remove_socket(_: web.Application) -> None:
+        unix_socket.unlink(missing_ok=True)
+
+    application.on_shutdown.append(remove_socket)
+
+    return sock
+
+
 async def on_shutdown(application: web.Application) -> None:
     """
     web application shutdown handler
@@ -78,9 +113,9 @@ def run_server(application: web.Application) -> None:
     configuration: Configuration = application["configuration"]
     host = configuration.get("web", "host")
     port = configuration.getint("web", "port")
-    unix_socket = configuration.get("web", "unix_socket", fallback=None)
+    unix_socket = create_socket(configuration, application)
 
-    web.run_app(application, host=host, port=port, path=unix_socket, handle_signals=False,
+    web.run_app(application, host=host, port=port, sock=unix_socket, handle_signals=False,
                 access_log=logging.getLogger("http"), access_log_class=FilteredAccessLogger)
 
 

tests/ahriman/application/test_ahriman.py

@@ -258,13 +258,14 @@ def test_subparsers_patch_add_architecture(parser: argparse.ArgumentParser) -> N
 
 def test_subparsers_patch_list(parser: argparse.ArgumentParser) -> None:
     """
-    patch-list command must imply action, architecture list, lock and report
+    patch-list command must imply action, architecture list, lock, report and unsafe
     """
     args = parser.parse_args(["patch-list", "ahriman"])
     assert args.action == Action.List
     assert args.architecture == [""]
     assert args.lock is None
     assert not args.report
+    assert args.unsafe
 
 
 def test_subparsers_patch_list_architecture(parser: argparse.ArgumentParser) -> None:
@@ -558,12 +559,13 @@ def test_subparsers_repo_sync_architecture(parser: argparse.ArgumentParser) -> N
 
 def test_subparsers_repo_tree(parser: argparse.ArgumentParser) -> None:
     """
-    repo-tree command must imply lock, report and quiet
+    repo-tree command must imply lock, report, quiet and unsafe
     """
     args = parser.parse_args(["repo-tree"])
     assert args.lock is None
     assert not args.report
     assert args.quiet
+    assert args.unsafe
 
 
 def test_subparsers_repo_tree_architecture(parser: argparse.ArgumentParser) -> None:
@@ -619,15 +621,14 @@ def test_subparsers_shell(parser: argparse.ArgumentParser) -> None:
 
 def test_subparsers_user_add(parser: argparse.ArgumentParser) -> None:
     """
-    user-add command must imply action, architecture, lock, report, quiet and unsafe
+    user-add command must imply action, architecture, lock, report and quiet
     """
     args = parser.parse_args(["user-add", "username"])
     assert args.action == Action.Update
     assert args.architecture == [""]
     assert args.lock is None
     assert not args.report
     assert args.quiet
-    assert args.unsafe
 
 
 def test_subparsers_user_add_architecture(parser: argparse.ArgumentParser) -> None:
@@ -680,7 +681,7 @@ def test_subparsers_user_list_option_role(parser: argparse.ArgumentParser) -> No
 
 def test_subparsers_user_remove(parser: argparse.ArgumentParser) -> None:
     """
-    user-remove command must imply action, architecture, lock, report, password, quiet, role and unsafe
+    user-remove command must imply action, architecture, lock, report, password and quiet
     """
     args = parser.parse_args(["user-remove", "username"])
     assert args.action == Action.Remove
@@ -689,7 +690,6 @@ def test_subparsers_user_remove(parser: argparse.ArgumentParser) -> None:
     assert not args.report
     assert args.password is not None
     assert args.quiet
-    assert args.unsafe
 
 
 def test_subparsers_user_remove_architecture(parser: argparse.ArgumentParser) -> None:

tests/ahriman/core/status/test_web_client.py

@@ -123,6 +123,9 @@ def test_package_url(web_client: WebClient, package_ahriman: Package) -> None:
     """
     must generate package status url correctly
     """
+    assert web_client._package_url("").startswith(web_client.address)
+    assert web_client._package_url("").endswith(f"/api/v1/packages")
+
     assert web_client._package_url(package_ahriman.base).startswith(web_client.address)
     assert web_client._package_url(package_ahriman.base).endswith(f"/api/v1/packages/{package_ahriman.base}")
 

tests/ahriman/web/test_web.py

@@ -1,12 +1,62 @@
 import pytest
+import socket
 
 from aiohttp import web
 from pytest_mock import MockerFixture
+from unittest.mock import call as MockCall
 
 from ahriman.core.exceptions import InitializeError
 from ahriman.core.log.filtered_access_logger import FilteredAccessLogger
 from ahriman.core.status.watcher import Watcher
-from ahriman.web.web import on_shutdown, on_startup, run_server
+from ahriman.web.web import create_socket, on_shutdown, on_startup, run_server
+
+
+async def test_create_socket(application: web.Application, mocker: MockerFixture) -> None:
+    """
+    must create socket
+    """
+    path = "/run/ahriman.sock"
+    application["configuration"].set_option("web", "unix_socket", str(path))
+    current_on_shutdown = len(application.on_shutdown)
+
+    bind_mock = mocker.patch("socket.socket.bind")
+    chmod_mock = mocker.patch("pathlib.Path.chmod")
+    unlink_mock = mocker.patch("pathlib.Path.unlink")
+
+    sock = create_socket(application["configuration"], application)
+    assert sock.family == socket.AF_UNIX
+    assert sock.type == socket.SOCK_STREAM
+    bind_mock.assert_called_once_with(str(path))
+    chmod_mock.assert_called_once_with(0o666)
+    assert len(application.on_shutdown) == current_on_shutdown + 1
+
+    # provoke socket removal
+    await application.on_shutdown[-1](application)
+    unlink_mock.assert_has_calls([MockCall(missing_ok=True), MockCall(missing_ok=True)])
+
+
+def test_create_socket_empty(application: web.Application) -> None:
+    """
+    must skip socket creation if not set by configuration
+    """
+    assert create_socket(application["configuration"], application) is None
+
+
+def test_create_socket_safe(application: web.Application, mocker: MockerFixture) -> None:
+    """
+    must create socket with default permission set
+    """
+    path = "/run/ahriman.sock"
+    application["configuration"].set_option("web", "unix_socket", str(path))
+    application["configuration"].set_option("web", "unix_socket_unsafe", "no")
+
+    mocker.patch("socket.socket.bind")
+    mocker.patch("pathlib.Path.unlink")
+    chmod_mock = mocker.patch("pathlib.Path.chmod")
+
+    sock = create_socket(application["configuration"], application)
+    assert sock is not None
+    chmod_mock.assert_not_called()
 
 
 async def test_on_shutdown(application: web.Application, mocker: MockerFixture) -> None:
@@ -50,7 +100,7 @@ def test_run(application: web.Application, mocker: MockerFixture) -> None:
 
     run_server(application)
     run_application_mock.assert_called_once_with(
-        application, host="127.0.0.1", port=port, path=None, handle_signals=False,
+        application, host="127.0.0.1", port=port, sock=None, handle_signals=False,
         access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger
     )
 
@@ -65,7 +115,7 @@ def test_run_with_auth(application_with_auth: web.Application, mocker: MockerFix
 
     run_server(application_with_auth)
     run_application_mock.assert_called_once_with(
-        application_with_auth, host="127.0.0.1", port=port, path=None, handle_signals=False,
+        application_with_auth, host="127.0.0.1", port=port, sock=None, handle_signals=False,
         access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger
     )
 
@@ -80,7 +130,7 @@ def test_run_with_debug(application_with_debug: web.Application, mocker: MockerF
 
     run_server(application_with_debug)
     run_application_mock.assert_called_once_with(
-        application_with_debug, host="127.0.0.1", port=port, path=None, handle_signals=False,
+        application_with_debug, host="127.0.0.1", port=port, sock=None, handle_signals=False,
         access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger
     )
 
@@ -90,13 +140,13 @@ def test_run_with_socket(application: web.Application, mocker: MockerFixture) ->
     must run application
     """
     port = 8080
-    socket = "/run/ahriman.sock"
     application["configuration"].set_option("web", "port", str(port))
-    application["configuration"].set_option("web", "unix_socket", socket)
+    socket_mock = mocker.patch("ahriman.web.web.create_socket", return_value=42)
     run_application_mock = mocker.patch("aiohttp.web.run_app")
 
     run_server(application)
+    socket_mock.assert_called_once_with(application["configuration"], application)
     run_application_mock.assert_called_once_with(
-        application, host="127.0.0.1", port=port, path=socket, handle_signals=False,
+        application, host="127.0.0.1", port=port, sock=42, handle_signals=False,
         access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger
     )