[ADD] runbot_merge: webhook signature support

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)

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']

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': {
+                'repository': self._repo(pr.repo.name),
-                    'name': pr.repo.name.split('/')[1],
-                    'full_name': 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': {
+                'repo': self._repo(pr.repo.name),
-                    'name': pr.repo.name.split('/')[1],
-                    'full_name': pr.repo.name,
-                },
             },
             'title': pr.title,
             'body': pr.body,

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

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'})