From 3ba36377687f8f49dc81e5db916e5c87e6603353 Mon Sep 17 00:00:00 2001 From: Latent Logic Date: Mon, 26 Nov 2018 21:43:26 -0800 Subject: [PATCH] Support slack signing secrets for verification Disables verification tokens if provided. --- sirbot/plugins/slack/endpoints.py | 48 ++++++++++--- sirbot/plugins/slack/plugin.py | 20 +++++- tests/test_plugin_slack.py | 116 +++++++++++++++++++++++++++++- 3 files changed, 170 insertions(+), 14 deletions(-) diff --git a/sirbot/plugins/slack/endpoints.py b/sirbot/plugins/slack/endpoints.py index e366faa..0806425 100644 --- a/sirbot/plugins/slack/endpoints.py +++ b/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,30 @@ 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: + if slack.signing_secret: + validate_request_signature( + await request.read(), request.headers, slack.signing_secret + ) + verification_token = None + else: + verification_token = slack.verify + event = Event.from_http(payload, verification_token=verification_token) + except (FailedVerification, InvalidSlackSignature, InvalidTimestamp): return Response(status=401) if event["type"] == "message": @@ -81,8 +97,15 @@ async def incoming_command(request): payload = await request.post() try: - command = Command(payload, verification_token=slack.verify) - except FailedVerification: + if slack.signing_secret: + validate_request_signature( + await request.read(), request.headers, slack.signing_secret + ) + verification_token = None + else: + verification_token = slack.verify + command = Command(payload, verification_token=verification_token) + except (FailedVerification, InvalidSlackSignature, InvalidTimestamp): return Response(status=401) LOG.debug("Incoming command: %s", command) @@ -99,8 +122,15 @@ 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: + if slack.signing_secret: + validate_request_signature( + await request.read(), request.headers, slack.signing_secret + ) + verification_token = None + else: + verification_token = slack.verify + action = Action.from_http(payload, verification_token=verification_token) + except (FailedVerification, InvalidSlackSignature, InvalidTimestamp): return Response(status=401) LOG.debug("Incoming action: %s", action) diff --git a/sirbot/plugins/slack/plugin.py b/sirbot/plugins/slack/plugin.py index c93437b..8393c56 100644 --- a/sirbot/plugins/slack/plugin.py +++ b/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 os.environ.get("SLACK_SIGNING_SECRET", False): + 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 = {} diff --git a/tests/test_plugin_slack.py b/tests/test_plugin_slack.py index 845b36f..53ea13b 100644 --- a/tests/test_plugin_slack.py +++ b/tests/test_plugin_slack.py @@ -1,6 +1,13 @@ import re +import hmac +import json +import time import asyncio +import hashlib +import urllib.parse +from typing import Dict, Tuple, Union, Optional from unittest import mock +from collections import MutableMapping import slack import pytest @@ -25,6 +32,49 @@ async def bot(): return b +@pytest.fixture +async def bot_signing(): + b = SirBot() + b.load_plugin( + SlackPlugin( + token="foo", + signing_secret="sharedsigningkey", + bot_user_id="baz", + bot_id="boo", + admins=["aaa", "bbb"], + ) + ) + return b + + +def _sign_body( + json_data: Optional[Dict] = None, + post_data: Optional[Dict] = None, + signing_secret: str = "sharedsigningkey", + timestamp: Optional[int] = None, +) -> Tuple[Dict[str, str], bytes]: + if json_data: + headers = {"content-type": "application/json"} + body = json.dumps(json_data).encode("utf-8") + elif post_data: + headers = {"content-type": "application/x-www-form-urlencoded"} + body = urllib.parse.urlencode(post_data).encode("utf-8") + else: + raise ValueError("Unknown type of data to sign") + if timestamp is None: + timestamp = int(time.time()) + headers["X-Slack-Request-Timestamp"] = str(timestamp) + headers["X-Slack-Signature"] = ( + "v0=" + + hmac.new( + signing_secret.encode("utf-8"), + f"""v0:{timestamp}:{body}""".encode("utf-8"), + digestmod=hashlib.sha256, + ).hexdigest() + ) + return headers, body + + @pytest.fixture def find_bot_id_query(): async def query(*args, **kwargs): @@ -213,16 +263,40 @@ async def test_incoming_event(self, bot, aiohttp_client, slack_event): r = await client.post("/slack/events", json=slack_event) assert r.status == 200 + async def test_incoming_event_signed( + self, bot_signing, aiohttp_client, slack_event + ): + client = await aiohttp_client(bot_signing) + headers, body = _sign_body(json_data=slack_event) + r = await client.post("/slack/events", headers=headers, data=body) + assert r.status == 200 + async def test_incoming_command(self, bot, aiohttp_client, slack_command): client = await aiohttp_client(bot) r = await client.post("/slack/commands", data=slack_command) assert r.status == 200 + async def test_incoming_command_signed( + self, bot_signing, aiohttp_client, slack_command + ): + client = await aiohttp_client(bot_signing) + headers, body = _sign_body(post_data=slack_command) + r = await client.post("/slack/commands", headers=headers, data=body) + assert r.status == 200 + async def test_incoming_action(self, bot, aiohttp_client, slack_action): client = await aiohttp_client(bot) r = await client.post("/slack/actions", data=slack_action) assert r.status == 200 + async def test_incoming_action_signed( + self, bot_signing, aiohttp_client, slack_action + ): + client = await aiohttp_client(bot_signing) + headers, body = _sign_body(post_data=slack_action) + r = await client.post("/slack/actions", headers=headers, data=body) + assert r.status == 200 + async def test_incoming_event_wrong_token(self, bot, aiohttp_client, slack_event): bot["plugins"]["slack"].verify = "bar" client = await aiohttp_client(bot) @@ -355,19 +429,57 @@ async def test_event_challenge(self, bot, aiohttp_client): client = await aiohttp_client(bot) r = await client.post( "/slack/events", - json={"token": "supersecuretoken", "challenge": "abcdefghij"}, + json={ + "token": "supersecuretoken", + "challenge": "abcdefghij", + "type": "url_verification", + }, ) data = await r.text() assert r.status == 200 assert data == "abcdefghij" + assert r.status == 200 + + async def test_event_challenge_signed(self, bot_signing, aiohttp_client): + + client = await aiohttp_client(bot_signing) + headers, body = _sign_body( + json_data={ + "token": "na", + "challenge": "abcdefghij", + "type": "url_verification", + } + ) + r = await client.post("/slack/events", data=body, headers=headers) + data = await r.text() + assert r.status == 200 + assert data == "abcdefghij" async def test_event_challenge_wrong_token(self, bot, aiohttp_client): client = await aiohttp_client(bot) r = await client.post( "/slack/events", - json={"token": "wrongsupersecuretoken", "challenge": "abcdefghij"}, + json={ + "token": "wrongsupersecuretoken", + "challenge": "abcdefghij", + "type": "url_verification", + }, + ) + assert r.status == 500 + + async def test_event_challenge_signed_wrong(self, bot_signing, aiohttp_client): + + client = await aiohttp_client(bot_signing) + headers, body = _sign_body( + json_data={ + "token": "na", + "challenge": "abcdefghij", + "type": "url_verification", + }, + signing_secret="notsharedsigningkey", ) + r = await client.post("/slack/events", data=body, headers=headers) assert r.status == 500 @pytest.mark.parametrize("slack_message", ("bot",), indirect=True)