Slack shared secret message signing & aiohttp deprecation warnings (#30)

* Fix aiohttp testing errors from aio-libs/aiohttp#2578

DeprecationWarning: Deprecated, use aiohttp_server fixture instead
DeprecationWarning: Deprecated, use aiohttp_client fixture instead

* Support slack signing secrets for verification

Disables verification tokens if provided.

sirbot/plugins/slack/endpoints.py

@@ -4,9 +4,10 @@
 import aiohttp.web
 from aiohttp.web import Response
 from slack.events import Event
+from slack.sansio import validate_request_signature
 from slack.actions import Action
 from slack.commands import Command
-from slack.exceptions import FailedVerification
+from slack.exceptions import InvalidTimestamp, FailedVerification, InvalidSlackSignature
 
 LOG = logging.getLogger(__name__)
 
@@ -16,15 +17,24 @@ async def incoming_event(request):
     payload = await request.json()
     LOG.log(5, "Incoming event payload: %s", payload)
 
-    if "challenge" in payload:
-        if payload["token"] == slack.verify:
+    if payload.get("type") == "url_verification":
+        if slack.signing_secret:
+            try:
+                validate_request_signature(
+                    await request.read(), request.headers, slack.signing_secret
+                )
+                return Response(body=payload["challenge"])
+            except (InvalidSlackSignature, InvalidTimestamp):
+                return Response(status=500)
+        elif payload["token"] == slack.verify:
             return Response(body=payload["challenge"])
         else:
             return Response(status=500)
 
     try:
-        event = Event.from_http(payload, verification_token=slack.verify)
-    except FailedVerification:
+        verification_token = await _validate_request(request, slack)
+        event = Event.from_http(payload, verification_token=verification_token)
+    except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):
         return Response(status=401)
 
     if event["type"] == "message":
@@ -81,8 +91,9 @@ async def incoming_command(request):
     payload = await request.post()
 
     try:
-        command = Command(payload, verification_token=slack.verify)
-    except FailedVerification:
+        verification_token = await _validate_request(request, slack)
+        command = Command(payload, verification_token=verification_token)
+    except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):
         return Response(status=401)
 
     LOG.debug("Incoming command: %s", command)
@@ -99,8 +110,9 @@ async def incoming_action(request):
     LOG.log(5, "Incoming action payload: %s", payload)
 
     try:
-        action = Action.from_http(payload, verification_token=slack.verify)
-    except FailedVerification:
+        verification_token = await _validate_request(request, slack)
+        action = Action.from_http(payload, verification_token=verification_token)
+    except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):
         return Response(status=401)
 
     LOG.debug("Incoming action: %s", action)
@@ -143,3 +155,13 @@ async def _wait_and_check_result(futures):
         return results[0]
 
     return Response(status=200)
+
+
+async def _validate_request(request, slack):
+    if slack.signing_secret:
+        validate_request_signature(
+            await request.read(), request.headers, slack.signing_secret
+        )
+        return None
+    else:
+        return slack.verify

sirbot/plugins/slack/plugin.py

@@ -24,10 +24,12 @@ class SlackPlugin:
 
     Args:
         token: slack authentication token (env var: `SLACK_TOKEN`).
-        verify: slack verification token (env var: `SLACK_VERIFY`).
         bot_id: bot id (env var: `SLACK_BOT_ID`).
         bot_user_id: user id of the bot (env var: `SLACK_BOT_USER_ID`).
         admins: list of slack admins user id (env var: `SLACK_ADMINS`).
+        verify: slack verification token (env var: `SLACK_VERIFY`).
+        signing_secret: slack signing secret key (env var: `SLACK_SIGNING_SECRET`).
+                        (disables verification token if provided).
 
     **Variables**:
         * **api**: Slack client. Instance of :class:`slack.io.aiohttp.SlackAPI`.
@@ -36,12 +38,24 @@ class SlackPlugin:
     __name__ = "slack"
 
     def __init__(
-        self, *, token=None, verify=None, bot_id=None, bot_user_id=None, admins=None
+        self,
+        *,
+        token=None,
+        bot_id=None,
+        bot_user_id=None,
+        admins=None,
+        verify=None,
+        signing_secret=None
     ):
         self.api = None
         self.token = token or os.environ["SLACK_TOKEN"]
         self.admins = admins or os.environ.get("SLACK_ADMINS", [])
-        self.verify = verify or os.environ["SLACK_VERIFY"]
+        if signing_secret or "SLACK_SIGNING_SECRET" in os.environ:
+            self.signing_secret = signing_secret or os.environ["SLACK_SIGNING_SECRET"]
+            self.verify = None
+        else:
+            self.verify = verify or os.environ["SLACK_VERIFY"]
+            self.signing_secret = None
         self.bot_id = bot_id or os.environ.get("SLACK_BOT_ID")
         self.bot_user_id = bot_user_id or os.environ.get("SLACK_BOT_USER_ID")
         self.handlers_option = {}

tests/test_plugin_apscheduler.py

@@ -11,6 +11,6 @@ async def bot():
 
 
 class TestPluginAPscheduler:
-    async def test_start(self, bot, test_server):
-        await test_server(bot)
+    async def test_start(self, bot, aiohttp_server):
+        await aiohttp_server(bot)
         assert isinstance(bot["plugins"]["scheduler"], APSchedulerPlugin)

tests/test_plugin_github.py

@@ -24,28 +24,28 @@ async def event(request):
 
 
 class TestPluginGithub:
-    async def test_start(self, bot, test_server):
-        await test_server(bot)
+    async def test_start(self, bot, aiohttp_server):
+        await aiohttp_server(bot)
         assert isinstance(bot["plugins"]["github"], GithubPlugin)
 
-    async def test_incoming_event(self, bot, test_client, event):
-        client = await test_client(bot)
+    async def test_incoming_event(self, bot, aiohttp_client, event):
+        client = await aiohttp_client(bot)
         r = await client.post("/github", json=event[0], headers=event[1])
         assert r.status == 200
 
-    async def test_incoming_event_401(self, bot, test_client, event):
+    async def test_incoming_event_401(self, bot, aiohttp_client, event):
         bot["plugins"]["github"].verify = "wrongsupersecrettoken"
-        client = await test_client(bot)
+        client = await aiohttp_client(bot)
         r = await client.post("/github", json=event[0], headers=event[1])
         assert r.status == 401
 
-    async def test_incoming_event_handler_error(self, bot, test_client, event):
+    async def test_incoming_event_handler_error(self, bot, aiohttp_client, event):
         async def handler(event, app):
             raise RuntimeError()
 
         bot["plugins"]["github"].router.add(
             handler, event[1]["X-GitHub-Event"], action=event[0]["action"]
         )
-        client = await test_client(bot)
+        client = await aiohttp_client(bot)
         r = await client.post("/github", json=event[0], headers=event[1])
         assert r.status == 500

tests/test_plugin_postgres.py

@@ -25,28 +25,28 @@ async def _teardown(self, bot):
             await pg_con.execute("""DROP SCHEMA IF EXISTS sirbot_test CASCADE""")
             await pg_con.execute("""DROP TABLE IF EXISTS metadata""")
 
-    async def test_start(self, bot, test_server):
+    async def test_start(self, bot, aiohttp_server):
         try:
-            await test_server(bot)
+            await aiohttp_server(bot)
             assert isinstance(bot["plugins"]["pg"], PgPlugin)
         finally:
             await self._teardown(bot)
 
-    async def test_no_migration(self, bot, test_server):
+    async def test_no_migration(self, bot, aiohttp_server):
         try:
             bot["plugins"]["pg"].sql_migration_directory = None
-            await test_server(bot)
+            await aiohttp_server(bot)
 
             with pytest.raises(asyncpg.exceptions.UndefinedTableError):
                 async with bot["plugins"]["pg"].connection() as pg_con:
                     await pg_con.fetchval("""SELECT db_version FROM metadata""")
         finally:
             await self._teardown(bot)
 
-    async def test_initial_migration(self, bot, test_server):
+    async def test_initial_migration(self, bot, aiohttp_server):
         try:
             bot["plugins"]["pg"].version = "0.0.1"
-            await test_server(bot)
+            await aiohttp_server(bot)
             async with bot["plugins"]["pg"].connection() as pg_con:
                 version = await pg_con.fetchval("""SELECT db_version FROM metadata""")
                 count = await pg_con.fetchval(
@@ -58,9 +58,9 @@ async def test_initial_migration(self, bot, test_server):
         finally:
             await self._teardown(bot)
 
-    async def test_migration_to_0_0_2(self, bot, test_server):
+    async def test_migration_to_0_0_2(self, bot, aiohttp_server):
         try:
-            await test_server(bot)
+            await aiohttp_server(bot)
 
             async with bot["plugins"]["pg"].connection() as pg_con:
                 version = await pg_con.fetchval("""SELECT db_version FROM metadata""")
@@ -82,10 +82,10 @@ async def test_migration_to_0_0_2(self, bot, test_server):
         finally:
             await self._teardown(bot)
 
-    async def test_no_migration_needed(self, bot, test_server):
+    async def test_no_migration_needed(self, bot, aiohttp_server):
         try:
             bot["plugins"]["pg"].version = "0.1.9"
-            await test_server(bot)
+            await aiohttp_server(bot)
 
             async with bot["plugins"]["pg"].connection() as pg_con:
                 count_start = await pg_con.fetchval(
@@ -104,12 +104,12 @@ async def test_no_migration_needed(self, bot, test_server):
         finally:
             await self._teardown(bot)
 
-    async def test_failed_migration(self, bot, test_server):
+    async def test_failed_migration(self, bot, aiohttp_server):
         try:
             bot["plugins"]["pg"].version = "0.2.0"
 
             with pytest.raises(asyncpg.exceptions.UndefinedColumnError):
-                await test_server(bot)
+                await aiohttp_server(bot)
 
             with pytest.raises(asyncpg.exceptions.UndefinedTableError):
                 async with bot["plugins"]["pg"].connection() as pg_con:

tests/test_plugin_readthedocs.py

@@ -12,8 +12,8 @@ async def bot():
 
 
 class TestPluginReadTheDocs:
-    async def test_start(self, bot, test_server):
-        await test_server(bot)
+    async def test_start(self, bot, aiohttp_server):
+        await aiohttp_server(bot)
         assert isinstance(bot["plugins"]["readthedocs"], RTDPlugin)
 
     async def test_register_project(self, bot):
@@ -134,7 +134,7 @@ def handler_bis():
         assert h[1] is handler_bis
         assert "test" in bot["plugins"]["readthedocs"]._projects
 
-    async def test_incoming(self, bot, test_client):
+    async def test_incoming(self, bot, aiohttp_client):
         async def handler(payload, app):
             assert payload == {
                 "build": {
@@ -147,7 +147,7 @@ async def handler(payload, app):
             }
             assert app is bot
 
-        client = await test_client(bot)
+        client = await aiohttp_client(bot)
         bot["plugins"]["readthedocs"].register_handler("sir-bot-a-lot", handler=handler)
 
         r = await client.post(
@@ -164,11 +164,11 @@ async def handler(payload, app):
         )
         assert r.status == 200
 
-    async def test_incoming_handler_error(self, bot, test_client):
+    async def test_incoming_handler_error(self, bot, aiohttp_client):
         async def handler(payload, app):
             raise RuntimeError()
 
-        client = await test_client(bot)
+        client = await aiohttp_client(bot)
         bot["plugins"]["readthedocs"].register_handler("sir-bot-a-lot", handler=handler)
 
         r = await client.post(
@@ -185,8 +185,8 @@ async def handler(payload, app):
         )
         assert r.status == 500
 
-    async def test_incoming_no_project(self, bot, test_client):
-        client = await test_client(bot)
+    async def test_incoming_no_project(self, bot, aiohttp_client):
+        client = await aiohttp_client(bot)
         r = await client.post(
             "/readthedocs",
             json={
@@ -201,8 +201,8 @@ async def test_incoming_no_project(self, bot, test_client):
         )
         assert r.status == 400
 
-    async def test_incoming_project_no_handler(self, bot, test_client):
-        client = await test_client(bot)
+    async def test_incoming_project_no_handler(self, bot, aiohttp_client):
+        client = await aiohttp_client(bot)
         bot["plugins"]["readthedocs"].register_project(
             "sir-bot-a-lot", build_url="https://example.com", jeton="aaaaaa"
         )
@@ -220,8 +220,8 @@ async def test_incoming_project_no_handler(self, bot, test_client):
         )
         assert r.status == 200
 
-    async def test_incoming_bad_json(self, bot, test_client):
-        client = await test_client(bot)
+    async def test_incoming_bad_json(self, bot, aiohttp_client):
+        client = await aiohttp_client(bot)
         r = await client.post("/readthedocs", json={"a": "b"})
         assert r.status == 400