Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Add support for Slack shared secret message signing #30

Merged
merged 3 commits into from
Nov 27, 2018
+277 −119
Merged

Add support for Slack shared secret message signing #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27138be
Fix aiohttp testing errors from aio-libs/aiohttp#2578
Latent-Logic Nov 27, 2018
895d55c
Support slack signing secrets for verification
Latent-Logic Nov 27, 2018
e7d8c1a
Fix Travis CI `black` failure for number formatting
Latent-Logic Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions sirbot/plugins/slack/endpoints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand All @@ -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":
Latent-Logic marked this conversation as resolved.
Show resolved Hide resolved
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code is used in multiple endpoint. I would be best to take it out in a function, something like

try:
    validate_request(request, slack.signing_secret)
    event = Event.from_http(payload, verification_token=slack.verify)
except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to explicitly keep verification_token as a new variable so the token we're passing into Event is explicitly None when the signing_secret is set. This way we're not relying on the initialization behavior in slack/plugin.py. If we wanted to rely on no one tampering with the slack.verify parameter then we could just call validate_request_signature rather than wrapping it in a validate_request function.

return Response(status=401)

if event["type"] == "message":
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
20 changes: 17 additions & 3 deletions sirbot/plugins/slack/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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 = {}
Expand Down
4 changes: 2 additions & 2 deletions tests/test_plugin_apscheduler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
16 changes: 8 additions & 8 deletions tests/test_plugin_github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
24 changes: 12 additions & 12 deletions tests/test_plugin_postgres.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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""")
Expand All @@ -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(
Expand All @@ -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:
Expand Down
24 changes: 12 additions & 12 deletions tests/test_plugin_readthedocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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": {
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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={
Expand All @@ -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"
)
Expand All @@ -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

Expand Down
Loading