From 6494ea6cb02517087eba50f319bff57c8c1d9469 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 13 Jun 2018 12:35:22 +0200 Subject: [PATCH] [ADD] runbot_merge: webhook signature support --- runbot_merge/controllers.py | 18 ++++++++- runbot_merge/models/pull_requests.py | 6 +++ runbot_merge/tests/fake_github/__init__.py | 37 ++++++++++++----- runbot_merge/tests/remote.py | 12 ++++++ runbot_merge/tests/test_basic.py | 47 ++++++++++++++++++++++ 5 files changed, 108 insertions(+), 12 deletions(-) diff --git a/runbot_merge/controllers.py b/runbot_merge/controllers.py index 8425a2a9..e9b6b381 100644 --- a/runbot_merge/controllers.py +++ b/runbot_merge/controllers.py @@ -1,6 +1,10 @@ +import hashlib +import hmac import logging import json +import werkzeug.exceptions + from odoo.http import Controller, request, route _logger = logging.getLogger(__name__) @@ -8,10 +12,22 @@ _logger = logging.getLogger(__name__) class MergebotController(Controller): @route('/runbot_merge/hooks', auth='none', type='json', csrf=False, methods=['POST']) def index(self): - event = request.httprequest.headers['X-Github-Event'] + req = request.httprequest + event = req.headers['X-Github-Event'] c = EVENTS.get(event) if c: + repo = request.jsonrequest['repository']['full_name'] + secret = request.env(user=1)['runbot_merge.repository'].search([ + ('name', '=', repo), + ]).project_id.secret + if secret: + signature = 'sha1=' + hmac.new(secret.encode('ascii'), req.get_data(), hashlib.sha1).hexdigest() + if not hmac.compare_digest(signature, req.headers.get('X-Hub-Signature', '')): + _logger.warn("Ignored hook with incorrect signature %s", + req.headers.get('X-Hub-Signature')) + return werkzeug.exceptions.Forbidden() + return c(request.jsonrequest) _logger.warn('Unknown event %s', event) return 'Unknown event {}'.format(event) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 87027f05..2f31efd1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -49,6 +49,12 @@ class Project(models.Model): batch_limit = fields.Integer( default=8, help="Maximum number of PRs staged together") + secret = fields.Char( + help="Webhook secret. If set, will be checked against the signature " + "of (valid) incoming webhook signatures, failing signatures " + "will lead to webhook rejection. Should only use ASCII." + ) + def _check_progress(self): logger = _logger.getChild('cron') Batch = self.env['runbot_merge.batch'] diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 86a91567..e99dd5bf 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/runbot_merge/tests/fake_github/__init__.py @@ -1,4 +1,6 @@ import collections +import hashlib +import hmac import io import json import logging @@ -96,6 +98,11 @@ class Repo(object): for client in self.hooks.get(event_type, []): getattr(client, event_type)(*payload) + def set_secret(self, secret): + for clients in self.hooks.values(): + for client in clients: + client.secret = secret + def issue(self, number): return self.issues[number] @@ -552,16 +559,28 @@ committer {} class Client(werkzeug.test.Client): def __init__(self, application, path): self._webhook_path = path + self.secret = None super(Client, self).__init__(application, werkzeug.wrappers.BaseResponse) def _make_env(self, event_type, data): + headers = [('X-Github-Event', event_type)] + body = json.dumps(data).encode('utf-8') + if self.secret: + sig = hmac.new(self.secret.encode('ascii'), body, hashlib.sha1).hexdigest() + headers.append(('X-Hub-Signature', 'sha1=' + sig)) + return werkzeug.test.EnvironBuilder( path=self._webhook_path, method='POST', - headers=[('X-Github-Event', event_type)], + headers=headers, content_type='application/json', - data=json.dumps(data), + data=body, ) + def _repo(self, name): + return { + 'name': name.split('/')[1], + 'full_name': name, + } def pull_request(self, action, pr, changes=None): assert action in ('opened', 'reopened', 'closed', 'synchronize', 'edited') @@ -569,6 +588,7 @@ class Client(werkzeug.test.Client): 'pull_request', { 'action': action, 'pull_request': self._pr(pr), + 'repository': self._repo(pr.repo.name), **({'changes': changes} if changes else {}) } )) @@ -589,10 +609,7 @@ class Client(werkzeug.test.Client): 'user': {'login': user}, }, 'pull_request': self._pr(pr), - 'repository': { - 'name': pr.repo.name.split('/')[1], - 'full_name': pr.repo.name, - } + 'repository': self._repo(pr.repo.name), } )) @@ -604,6 +621,7 @@ class Client(werkzeug.test.Client): 'context': context, 'state': state, 'sha': sha, + 'repository': self._repo(repository), } )) @@ -611,7 +629,7 @@ class Client(werkzeug.test.Client): contents = { 'action': 'created', 'issue': { 'number': issue.number }, - 'repository': { 'name': issue.repo.name.split('/')[1], 'full_name': issue.repo.name }, + 'repository': self._repo(issue.repo.name), 'sender': { 'login': user }, 'comment': { 'body': body }, } @@ -631,10 +649,7 @@ class Client(werkzeug.test.Client): }, 'base': { 'ref': pr.base, - 'repo': { - 'name': pr.repo.name.split('/')[1], - 'full_name': pr.repo.name, - }, + 'repo': self._repo(pr.repo.name), }, 'title': pr.title, 'body': pr.body, diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index 63a95b00..02b53b00 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -398,6 +398,18 @@ class Repo: self._session = session self._tokens = user_tokens + def set_secret(self, secret): + r = self._session.get( + 'https://api.github.com/repos/{}/hooks'.format(self.name)) + response = r.json() + assert 200 <= r.status_code < 300, response + [hook] = response + + r = self._session.patch('https://api.github.com/repos/{}/hooks/{}'.format(self.name, hook['id']), json={ + 'config': {**hook['config'], 'secret': secret}, + }) + assert 200 <= r.status_code < 300, r.json() + def get_ref(self, ref): if re.match(r'[0-9a-f]{40}', ref): return ref diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ebdab1a5..6410ed81 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -60,6 +60,53 @@ def test_trivial_flow(env, repo): } assert master.message, "gibberish\n\nblahblah\n\ncloses odoo/odoo#1" +class TestWebhookSecurity: + def test_no_secret(self, env, project, repo): + """ Test 1: didn't add a secret to the repo, should be ignored + """ + project.secret = "a secret" + + m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) + repo.make_ref('heads/master', m) + + c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) + pr0 = repo.make_pr("gibberish", "blahblah", target='master', ctid=c0, user='user') + + assert not env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr0.number), + ]) + + def test_wrong_secret(self, env, project, repo): + repo.set_secret("wrong secret") + project.secret = "a secret" + + m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) + repo.make_ref('heads/master', m) + + c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) + pr0 = repo.make_pr("gibberish", "blahblah", target='master', ctid=c0, user='user') + + assert not env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr0.number), + ]) + + def test_correct_secret(self, env, project, repo): + repo.set_secret("a secret") + project.secret = "a secret" + + m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) + repo.make_ref('heads/master', m) + + c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) + pr0 = repo.make_pr("gibberish", "blahblah", target='master', ctid=c0, user='user') + + assert env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr0.number), + ]) + def test_staging_conflict(env, repo): # create base branch m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})