From 74ed15e0aa06528b29237c599ea09177ff7cc763 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Wed, 13 Jan 2021 11:52:42 +0200 Subject: [PATCH] Update the signal integration to use DBus The initial implementation was just calling signal-cli directly using `subprocess.run`. Going with DBus makes it easier to shield signal-cli from the rest of the system. It also makes sure the signal-cli daemon is running in the background and receiving messages. This is important when a recipient does the "Reset secure connection" from the app. We must receive their new keys, otherwise our future messages will appear as "bad encrypted message" for them. --- README.md | 33 ++++--------------- hc/api/tests/test_notify_signal.py | 52 +++++++++++++++--------------- hc/api/transports.py | 30 ++++++++++------- hc/front/tests/test_add_signal.py | 4 +-- hc/front/views.py | 6 ++-- hc/settings.py | 3 +- 6 files changed, 56 insertions(+), 72 deletions(-) diff --git a/README.md b/README.md index 3009c9f1..02deb566 100644 --- a/README.md +++ b/README.md @@ -136,8 +136,7 @@ Healthchecks reads configuration from the following environment variables: | PUSHOVER_SUBSCRIPTION_URL | `None` | REMOTE_USER_HEADER | `None` | See [External Authentication](#external-authentication) for details. | SHELL_ENABLED | `"False"` -| SIGNAL_CLI_USERNAME | `None` -| SIGNAL_CLI_CMD | `signal-cli` | Path to the signal-cli executable +| SIGNAL_CLI_ENABLED | `"False"` | SLACK_CLIENT_ID | `None` | SLACK_CLIENT_SECRET | `None` | TELEGRAM_BOT_NAME | `"ExampleBot"` @@ -412,34 +411,14 @@ To enable the Pushover integration, you will need to: ### Signal Healthchecks uses [signal-cli](https://github.com/AsamK/signal-cli) to send Signal -notifications. It requires the `signal-cli` program to be installed and available on -the local machine. - -To send notifications, healthchecks executes "signal-cli send" calls. -It does not handle phone number registration and verification. You must do that -manually, before using the integration. +notifications. Healthcecks interacts with signal-cli over DBus. To enable the Signal integration: -* Download and install signal-cli in your preferred location - (for example, in `/srv/signal-cli-0.7.2/`). -* Register and verify phone number, or [link it](https://github.com/AsamK/signal-cli/wiki/Linking-other-devices-(Provisioning)) - to an existing registration. -* Test your signal-cli configuration by sending a message manually from command line. -* Put the sender phone number in the `SIGNAL_CLI_USERNAME` environment variable. - Example: `SIGNAL_CLI_USERNAME=+123456789`. -* If `signal-cli` is not in the system path, specify its path in `SIGNAL_CLI_CMD`. - Example: `SIGNAL_CLI_CMD=/srv/signal-cli-0.7.2/bin/signal-cli` - -It is possible to use a separate system user for running signal-cli: - -* Create a separate system user, (for example, "signal-user"). -* Configure signal-cli while logged in as signal-user. -* Change `SIGNAL_CLI_CMD` to run signal-cli through sudo: - `sudo -u signal-user /srv/signal-cli-0.7.2/bin/signal-cli`. -* Configure sudo to not require password. For example, if healthchecks - runs under the www-data system user, the sudoers rule would be: - `www-data ALL=(signal-user) NOPASSWD: /srv/signal-cli-0.7.2/bin/signal-cli`. +* Set up and configure signal-cli to listen on DBus system bus ([instructions](https://github.com/AsamK/signal-cli/wiki/DBus-service)). + Make sure you can send test messages from command line, using the `dbus-send` + example given in the signal-cli instructions. +* Set the `SIGNAL_CLI_ENABLED` environment variable to `True`. ### Telegram diff --git a/hc/api/tests/test_notify_signal.py b/hc/api/tests/test_notify_signal.py index 6b2d5eef..2ac57633 100644 --- a/hc/api/tests/test_notify_signal.py +++ b/hc/api/tests/test_notify_signal.py @@ -28,24 +28,22 @@ class NotifySignalTestCase(BaseTestCase): self.channel.save() self.channel.checks.add(self.check) - @patch("hc.api.transports.subprocess.run") - def test_it_works(self, mock_run): - mock_run.return_value.returncode = 0 - + @patch("hc.api.transports.dbus") + @patch("hc.api.transports.Signal.get_service") + def test_it_works(self, mock_get_service, mock_dbus): self.channel.notify(self.check) n = Notification.objects.get() self.assertEqual(n.error, "") - self.assertTrue(mock_run.called) - args, kwargs = mock_run.call_args - cmd = " ".join(args[0]) - - self.assertIn("-u +987654321", cmd) - self.assertIn("send +123456789", cmd) + self.assertTrue(mock_get_service.called) + args, kwargs = mock_get_service.return_value.sendMessage.call_args + self.assertIn("is DOWN", args[0]) + self.assertEqual(args[2], ["+123456789"]) - @patch("hc.api.transports.subprocess.run") - def test_it_obeys_down_flag(self, mock_run): + @patch("hc.api.transports.dbus") + @patch("hc.api.transports.Signal.get_service") + def test_it_obeys_down_flag(self, mock_get_service, mock_dbus): payload = {"value": "+123456789", "up": True, "down": False} self.channel.value = json.dumps(payload) self.channel.save() @@ -54,35 +52,35 @@ class NotifySignalTestCase(BaseTestCase): # This channel should not notify on "down" events: self.assertEqual(Notification.objects.count(), 0) - self.assertFalse(mock_run.called) - @patch("hc.api.transports.subprocess.run") - def test_it_requires_signal_cli_username(self, mock_run): + self.assertFalse(mock_get_service.called) - with override_settings(SIGNAL_CLI_USERNAME=None): + @patch("hc.api.transports.dbus") + @patch("hc.api.transports.Signal.get_service") + def test_it_requires_signal_cli_enabled(self, mock_get_service, mock_dbus): + with override_settings(SIGNAL_CLI_ENABLED=False): self.channel.notify(self.check) n = Notification.objects.get() self.assertEqual(n.error, "Signal notifications are not enabled") - self.assertFalse(mock_run.called) + self.assertFalse(mock_get_service.called) - @patch("hc.api.transports.subprocess.run") - def test_it_does_not_escape_special_characters(self, mock_run): + @patch("hc.api.transports.dbus") + @patch("hc.api.transports.Signal.get_service") + def test_it_does_not_escape_special_characters(self, mock_get_service, mock_dbus): self.check.name = "Foo & Bar" self.check.save() - mock_run.return_value.returncode = 0 self.channel.notify(self.check) - self.assertTrue(mock_run.called) - args, kwargs = mock_run.call_args - cmd = " ".join(args[0]) - - self.assertIn("Foo & Bar", cmd) + args, kwargs = mock_get_service.return_value.sendMessage.call_args + self.assertIn("Foo & Bar", args[0]) @override_settings(SECRET_KEY="test-secret") - def test_it_obeys_rate_limit(self): + @patch("hc.api.transports.dbus") + @patch("hc.api.transports.Signal.get_service") + def test_it_obeys_rate_limit(self, mock_get_service, mock_dbus): # "2862..." is sha1("+123456789test-secret") obj = TokenBucket(value="signal-2862991ccaa15c8856e7ee0abaf3448fb3c292e0") obj.tokens = 0 @@ -91,3 +89,5 @@ class NotifySignalTestCase(BaseTestCase): self.channel.notify(self.check) n = Notification.objects.first() self.assertEqual(n.error, "Rate limit exceeded") + + self.assertFalse(mock_get_service.called) diff --git a/hc/api/transports.py b/hc/api/transports.py index c5786df0..79a53cbd 100644 --- a/hc/api/transports.py +++ b/hc/api/transports.py @@ -6,7 +6,6 @@ from django.utils import timezone from django.utils.html import escape import json import requests -import subprocess from urllib.parse import quote, urlencode from hc.accounts.models import Profile @@ -19,6 +18,12 @@ except ImportError: # Enforce settings.APPRISE_ENABLED = False +try: + import dbus +except ImportError: + # Enforce + settings.SIGNAL_CLI_ENABLED = False + def tmpl(template_name, **ctx): template_path = "integrations/%s" % template_name @@ -669,8 +674,13 @@ class Signal(Transport): else: return not self.channel.signal_notify_up + def get_service(self): + bus = dbus.SystemBus() + signal_object = bus.get_object("org.asamk.Signal", "/org/asamk/Signal") + return dbus.Interface(signal_object, "org.asamk.Signal") + def notify(self, check): - if not settings.SIGNAL_CLI_USERNAME: + if not settings.SIGNAL_CLI_ENABLED: return "Signal notifications are not enabled" from hc.api.models import TokenBucket @@ -680,14 +690,10 @@ class Signal(Transport): text = tmpl("signal_message.html", check=check, site_name=settings.SITE_NAME) - args = settings.SIGNAL_CLI_CMD.split() - args.extend(["-u", settings.SIGNAL_CLI_USERNAME]) - args.extend(["send", self.channel.phone_number]) - args.extend(["-m", text]) - - # Need a high timeout because sending the first message to a new - # recipient sometimes takes 20+ seconds - result = subprocess.run(args, timeout=30) + try: + self.get_service().sendMessage(text, [], [self.channel.phone_number]) + except dbus.exceptions.DBusException as e: + if "NotFoundException" in str(e): + return "Recipient not found" - if result.returncode != 0: - return "signal-cli returned exit code %d" % result.returncode + return "signal-cli call failed" diff --git a/hc/front/tests/test_add_signal.py b/hc/front/tests/test_add_signal.py index 07642ec9..70fc4919 100644 --- a/hc/front/tests/test_add_signal.py +++ b/hc/front/tests/test_add_signal.py @@ -45,8 +45,8 @@ class AddSignalTestCase(BaseTestCase): self.assertFalse(c.signal_notify_down) self.assertFalse(c.signal_notify_up) - @override_settings(SIGNAL_CLI_USERNAME=None) - def test_it_handles_unset_sender_username(self): + @override_settings(SIGNAL_CLI_ENABLED=False) + def test_it_handles_disabled_integration(self): self.client.login(username="alice@example.org", password="password") r = self.client.get(self.url) self.assertEqual(r.status_code, 404) diff --git a/hc/front/views.py b/hc/front/views.py index efabd53b..1442e2e7 100644 --- a/hc/front/views.py +++ b/hc/front/views.py @@ -299,7 +299,7 @@ def index(request): "enable_pushbullet": settings.PUSHBULLET_CLIENT_ID is not None, "enable_pushover": settings.PUSHOVER_API_TOKEN is not None, "enable_shell": settings.SHELL_ENABLED is True, - "enable_signal": settings.SIGNAL_CLI_USERNAME is not None, + "enable_signal": settings.SIGNAL_CLI_ENABLED is True, "enable_slack_btn": settings.SLACK_CLIENT_ID is not None, "enable_sms": settings.TWILIO_AUTH is not None, "enable_telegram": settings.TELEGRAM_TOKEN is not None, @@ -763,7 +763,7 @@ def channels(request, code): "enable_pushbullet": settings.PUSHBULLET_CLIENT_ID is not None, "enable_pushover": settings.PUSHOVER_API_TOKEN is not None, "enable_shell": settings.SHELL_ENABLED is True, - "enable_signal": settings.SIGNAL_CLI_USERNAME is not None, + "enable_signal": settings.SIGNAL_CLI_ENABLED is True, "enable_slack_btn": settings.SLACK_CLIENT_ID is not None, "enable_sms": settings.TWILIO_AUTH is not None, "enable_telegram": settings.TELEGRAM_TOKEN is not None, @@ -1628,7 +1628,7 @@ def add_whatsapp(request, code): return render(request, "integrations/add_whatsapp.html", ctx) -@require_setting("SIGNAL_CLI_USERNAME") +@require_setting("SIGNAL_CLI_ENABLED") @login_required def add_signal(request, code): project = _get_rw_project_for_user(request, code) diff --git a/hc/settings.py b/hc/settings.py index daa9664a..0b3ee4d6 100644 --- a/hc/settings.py +++ b/hc/settings.py @@ -231,8 +231,7 @@ LINENOTIFY_CLIENT_ID = os.getenv("LINENOTIFY_CLIENT_ID") LINENOTIFY_CLIENT_SECRET = os.getenv("LINENOTIFY_CLIENT_SECRET") # Signal -SIGNAL_CLI_USERNAME = os.getenv("SIGNAL_CLI_USERNAME") -SIGNAL_CLI_CMD = os.getenv("SIGNAL_CLI_CMD", "signal-cli") +SIGNAL_CLI_ENABLED = envbool("SIGNAL_CLI_ENABLED", "False") if os.path.exists(os.path.join(BASE_DIR, "hc/local_settings.py")): from .local_settings import *