From 719631a5643b91d9755c6de84f9032d490f7d646 Mon Sep 17 00:00:00 2001 From: Josh Holbrook Date: Sat, 8 Feb 2025 16:50:27 -0900 Subject: [PATCH 1/5] dbus methods marked as unprivileged --- CHANGELOG.md | 4 ++++ plusdeck/dbus/interface.py | 21 +++++++++++---------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c487432..f62e431 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +yyyy/mm/dd Version 4.0.1-1 +-------------------------- +- Dbus methods now marked as unprivileged + 2025/02/06 Version 4.0.0-1 -------------------------- - `plusdeckctl` connects to the system bus by default diff --git a/plusdeck/dbus/interface.py b/plusdeck/dbus/interface.py index bacc607..9057b19 100644 --- a/plusdeck/dbus/interface.py +++ b/plusdeck/dbus/interface.py @@ -7,6 +7,7 @@ dbus_property_async, dbus_signal_async, DbusInterfaceCommonAsync, + DbusUnprivilegedFlag, ) from plusdeck.client import Client, create_connection, Receiver, State @@ -88,14 +89,14 @@ def closed(self: Self) -> asyncio.Future: return self.client.closed - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def play_a(self: Self) -> None: """ Play side A. """ self.client.play_a() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def play_b(self: Self) -> None: """ Play side B. @@ -103,7 +104,7 @@ async def play_b(self: Self) -> None: self.client.play_b() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def fast_forward_a(self: Self) -> None: """ Fast-forward side A. @@ -111,7 +112,7 @@ async def fast_forward_a(self: Self) -> None: self.client.fast_forward_a() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def fast_forward_b(self: Self) -> None: """ Fast-forward side B. @@ -119,7 +120,7 @@ async def fast_forward_b(self: Self) -> None: self.client.fast_forward_b() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def rewind_a(self: Self) -> None: """ Rewind side A. Equivalent to fast-forwarding side B. @@ -127,7 +128,7 @@ async def rewind_a(self: Self) -> None: self.client.rewind_a() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def rewind_b(self: Self) -> None: """ Rewind side B. Equivalent to fast-forwarding side A. @@ -135,7 +136,7 @@ async def rewind_b(self: Self) -> None: self.client.rewind_b() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def pause(self: Self) -> None: """ Pause if playing, or start playing if paused. @@ -143,7 +144,7 @@ async def pause(self: Self) -> None: self.client.pause() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def stop(self: Self) -> None: """ Stop the tape. @@ -151,7 +152,7 @@ async def stop(self: Self) -> None: self.client.stop() - @dbus_method_async("") + @dbus_method_async("", flags=DbusUnprivilegedFlag) async def eject(self: Self) -> None: """ Eject the tape. @@ -159,7 +160,7 @@ async def eject(self: Self) -> None: self.client.eject() - @dbus_method_async("sd", "b") + @dbus_method_async("sd", "b", flags=DbusUnprivilegedFlag) async def wait_for(self: Self, state: str, timeout: float) -> bool: """ Wait for an expected state, with an optional timeout. When timeout is negative, From f82d4a868cd6a4104507a4fbb54ccd67ce8f4221 Mon Sep 17 00:00:00 2001 From: Josh Holbrook Date: Sat, 8 Feb 2025 16:53:34 -0900 Subject: [PATCH 2/5] Remove polkit stuff --- docs/dbus.md | 24 +----------------------- polkit/org.jfhbrook.plusdeck.policy | 14 -------------- polkit/org.jfhbrook.plusdeck.rules | 14 -------------- 3 files changed, 1 insertion(+), 51 deletions(-) delete mode 100644 polkit/org.jfhbrook.plusdeck.policy delete mode 100644 polkit/org.jfhbrook.plusdeck.rules diff --git a/docs/dbus.md b/docs/dbus.md index ade95ca..1ee1f3c 100644 --- a/docs/dbus.md +++ b/docs/dbus.md @@ -53,7 +53,7 @@ The interface is similar to the vanilla `plusdeck` CLI. However, there are a few ## Dbus Access Policies -**NOTE: Full access for `plusdeck` group access is an area of active development. This feature does not work - at least, on Fedora.** To follow along, view [this StackExchange post](https://unix.stackexchange.com/questions/790750/dbus-policy-that-allows-group-to-access-system-service). and this [Fedora discussion post](https://discussion.fedoraproject.org/t/dbus-policy-that-allows-group-to-access-system-service/144265). +**NOTE: Full access for `plusdeck` group access is an area of active development. This feature may not work - at least, on Fedora.** To follow along, view [this StackExchange post](https://unix.stackexchange.com/questions/790750/dbus-policy-that-allows-group-to-access-system-service). and this [Fedora discussion post](https://discussion.fedoraproject.org/t/dbus-policy-that-allows-group-to-access-system-service/144265). When running services under the `system` bus, care must be taken to manage access policies. Dbus does this primarily with [an XML-based policy language](https://dbus.freedesktop.org/doc/dbus-daemon.1.html). Systemd additionally manages access to privileged methods, seemingly with the intent of delegating to polkit. @@ -69,12 +69,6 @@ sudo groupadd plusdeck sudo usermod -a -G plusdeck "${USER}" ``` -### Polkit - -**NOTE: The Polkit policies have not been shown to work at this time.** - -Prototype Polkit policies/rules may be found in the `./polkit` folder. - ## Running `plusdeckd` Directly The DBus service can be launched directly using `plusdeckd`: @@ -123,19 +117,3 @@ I have a just task for that: ```sh just get-dbus-iface ``` - -### Debugging SELinux - -While I haven't seen this to be the case, it seems theoretically possible for SELinux to block access to Dbus. - -You should be able to see access denials due to SELinux by running either: - -```sh -sudo ausearch -ts recent -``` - -or: - -```sh -sudo tail -f /var/log/audit/audit.log -``` diff --git a/polkit/org.jfhbrook.plusdeck.policy b/polkit/org.jfhbrook.plusdeck.policy deleted file mode 100644 index 75bf3df..0000000 --- a/polkit/org.jfhbrook.plusdeck.policy +++ /dev/null @@ -1,14 +0,0 @@ - - - - plusdeck - https://github.com/jfhbrook/plusdeck - - Polkit no allow eject tho - - yes - yes - yes - - - diff --git a/polkit/org.jfhbrook.plusdeck.rules b/polkit/org.jfhbrook.plusdeck.rules deleted file mode 100644 index 13865f4..0000000 --- a/polkit/org.jfhbrook.plusdeck.rules +++ /dev/null @@ -1,14 +0,0 @@ -polkit.addRule(function(action, subject) { - if ((action.id == "org.jfhbrook.plusdeck.Eject" || - action.id == "org.jfhbrook.plusdeck.FastForwardA" || - action.id == "org.jfhbrook.plusdeck.Pause" || - action.id == "org.jfhbrook.plusdeck.PlayA" || - action.id == "org.jfhbrook.plusdeck.PlayB" || - action.id == "org.jfhbrook.plusdeck.Stop" || - action.id == "org.jfhbrook.plusdeck.WaitFor") && - subject.isInGroup("plusdeck")) { - return polkit.Result.YES; - } - - return polkit.Result.NOT_HANDLED; -}); From 10eb5c869c6021367cbc1de1dfacc3709eb88f5c Mon Sep 17 00:00:00 2001 From: Josh Holbrook Date: Sun, 9 Feb 2025 11:12:09 -0900 Subject: [PATCH 3/5] Handle sdbus internal errors --- plusdeck/dbus/client.py | 13 ++----------- plusdeck/dbus/error.py | 38 ++++++++++++++++++++++++++++++++++++++ plusdeck/dbus/service.py | 6 ++++-- 3 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 plusdeck/dbus/error.py diff --git a/plusdeck/dbus/client.py b/plusdeck/dbus/client.py index a199e4b..8ef5d36 100644 --- a/plusdeck/dbus/client.py +++ b/plusdeck/dbus/client.py @@ -18,6 +18,7 @@ from plusdeck.client import State from plusdeck.config import Config from plusdeck.dbus.config import StagedConfig +from plusdeck.dbus.error import handle_dbus_error from plusdeck.dbus.interface import DBUS_NAME, DbusInterface logger = logging.getLogger(__name__) @@ -72,18 +73,8 @@ def pass_client(fn: AsyncCommand) -> AsyncCommand: @click.pass_obj @functools.wraps(fn) async def wrapped(obj: Obj, *args, **kwargs) -> None: - try: + async with handle_dbus_error(logger): await fn(obj.client, *args, **kwargs) - except Exception as exc: - if hasattr(exc, "dbus_error_name"): - dbus_error_name = cast(Any, exc).dbus_error_name - dbus_msg = str(exc) - if dbus_msg: - logger.error(f"{dbus_error_name}: {dbus_msg}") - else: - logger.error(f"{dbus_error_name}") - sys.exit(1) - raise return wrapped diff --git a/plusdeck/dbus/error.py b/plusdeck/dbus/error.py new file mode 100644 index 0000000..ce71bad --- /dev/null +++ b/plusdeck/dbus/error.py @@ -0,0 +1,38 @@ +from contextlib import asynccontextmanager +from logging import Logger +import re +import sys +from traceback import format_exc +from typing import Any, AsyncGenerator, cast, List, Optional + +from sdbus.sd_bus_internals import SdBusLibraryError + +ERROR_NUMBER_RE = r"returned error number: (\d+)" + + +@asynccontextmanager +async def handle_dbus_error(logger: Logger) -> AsyncGenerator[None, None]: + try: + yield + except Exception as exc: + exit_code: Optional[int] = None + if isinstance(exc, SdBusLibraryError): + logger.debug(format_exc()) + + error_numbers: List[str] = re.findall(ERROR_NUMBER_RE, str(exc)) + exit_code = int(error_numbers[0]) if error_numbers else 1 + + logger.error(f"SdBusLibraryError: {exc}") + elif hasattr(exc, "dbus_error_name"): + exit_code = 1 + dbus_error_name = cast(Any, exc).dbus_error_name + dbus_msg = str(exc) + if dbus_msg: + logger.error(f"{dbus_error_name}: {dbus_msg}") + else: + logger.error(f"{dbus_error_name}") + + if exit_code is not None: + sys.exit(exit_code) + + raise exc diff --git a/plusdeck/dbus/service.py b/plusdeck/dbus/service.py index 8a5dcc2..7df5621 100644 --- a/plusdeck/dbus/service.py +++ b/plusdeck/dbus/service.py @@ -10,6 +10,7 @@ from plusdeck.cli import LogLevel from plusdeck.config import GLOBAL_FILE +from plusdeck.dbus.error import handle_dbus_error from plusdeck.dbus.interface import DBUS_NAME, DbusInterface, load_client logger = logging.getLogger(__name__) @@ -40,9 +41,10 @@ async def serve(config_file: Optional[str] = None) -> None: Create and serve configure DBus service with a supplied config file. """ - srv = await service(config_file) + async with handle_dbus_error(logger): + srv = await service(config_file) - await srv.closed + await srv.closed @click.command From 1eddb89ef7dbfd7e51e5d91ef10e09ad6f99f87b Mon Sep 17 00:00:00 2001 From: Josh Holbrook Date: Sun, 9 Feb 2025 11:14:19 -0900 Subject: [PATCH 4/5] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f62e431..5fd8779 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ yyyy/mm/dd Version 4.0.1-1 -------------------------- - Dbus methods now marked as unprivileged +- sdbus library errors have improved logging 2025/02/06 Version 4.0.0-1 -------------------------- From dc5d01341be6fb9863b343c50b81998f62bc52cc Mon Sep 17 00:00:00 2001 From: Josh Holbrook Date: Sun, 9 Feb 2025 11:14:36 -0900 Subject: [PATCH 5/5] Greater confidence in dbus docs --- docs/dbus.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/dbus.md b/docs/dbus.md index 1ee1f3c..9085860 100644 --- a/docs/dbus.md +++ b/docs/dbus.md @@ -53,8 +53,6 @@ The interface is similar to the vanilla `plusdeck` CLI. However, there are a few ## Dbus Access Policies -**NOTE: Full access for `plusdeck` group access is an area of active development. This feature may not work - at least, on Fedora.** To follow along, view [this StackExchange post](https://unix.stackexchange.com/questions/790750/dbus-policy-that-allows-group-to-access-system-service). and this [Fedora discussion post](https://discussion.fedoraproject.org/t/dbus-policy-that-allows-group-to-access-system-service/144265). - When running services under the `system` bus, care must be taken to manage access policies. Dbus does this primarily with [an XML-based policy language](https://dbus.freedesktop.org/doc/dbus-daemon.1.html). Systemd additionally manages access to privileged methods, seemingly with the intent of delegating to polkit. By default, Dbus is configured with the following policies: