zuul
/
zuul
Code Issues Proposed changes

Pagure: remove connectors burden and simplify code 

This patch removes the use of the connector system. Indeed I've
figured out that user API token can be set with the needed
rights: pull_request_merge, pull_request_flag, pull_request_comment.
In fact, the default Pagure configuration (to be set by Pagure
operators) does not allow those right. Then it is just a matter
of configuration.

Recently pagure.io and src.fedoraproject.org operators have allowed
those rights for user API token.

Thus, I think, the connectors system (to get project scoped API token)
was a complex workaround to fix a configuration issue. I don't see
any reason for a Pagure operator to not allow those rights in the
user API token. Then, with this change, it is assumed that this
driver will be used with a Pagure instance that allow those ACL
rights.

In addition, this patch add an optional mechanism to allow webhook
payload according to the source of the events. This relies on a
white list of IP addresses from which an event will be accepted.
This option should only be used for testing or debugging purpose.

This change has several benefit:
- Less API call to Pagure
- Simplify driver code base

Change-Id: Ic38a6774dc4cb9798840a2c874e2d6e9ce16a067
This commit is contained in:
Fabien Boucher 2019-11-26 18:09:23 +01:00
parent 2106a72691
commit 0a614b1a9f
5 changed files with 162 additions and 180 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
doc/source/reference/drivers/pagure.rst
View File

@ -12,12 +12,29 @@ installations of Pagure.
Configure Pagure
----------------
Pagure's project owner must give project Admin access to the Pagure's user
that own the API key defined in the Zuul configuration. The API key
must at least have the ``Modify an existing project`` access.
The user's API token configured in zuul.conf must have the following
ACL rights:
Furthermore Project owner must set the web hook target url in project settings
such as: ``http://<zuul-web>/zuul/api/connection/<conn-name>/payload``
- "Merge a pull-request" set to on (optional, only for gating)
- "Flag a pull-request" set to on
- "Comment on a pull-request" set to on
- "Modify an existing project" set to on
Each project to be integrated with Zuul needs:
- "Web hook target" set to
http://<zuul-web>/zuul/api/connection/<conn-name>/payload
- "Notify on pull-request flag" set to on
- "Pull requests" set to on
- "Open metadata access to all" set to off (optional, expected if approval
based on PR a metadata tag)
- "Minimum score to merge pull-request" set to the same value than
the score requierement (optional, expected if score requierement is
defined in a pipeline)
Furthermore, the user must be added as project collaborator **admin** to
be able to read the project's webhook token. This token is used
to validate webhook's payload.
Connection Configuration
------------------------
@ -64,6 +81,18 @@ The supported options in ``zuul.conf`` connections are:
Path to the Pagure Git repositories. Used to clone.
.. attr:: source_whitelist
:default: ''
A comma separated list of source ip adresses from which webhook
calls are whitelisted. If the source is not whitelisted, then
call payload's signature is verified using the project webhook
token. An admin access to the project is required by Zuul to read
the token. White listing a source of hook calls allows Zuul to
react to events without any authorizations. This setting should
not be used in production.
Trigger Configuration
---------------------
Pagure webhook events can be configured as triggers.

28
tests/base.py
View File

@ -1318,9 +1318,9 @@ class FakePagureAPIClient(pagureconnection.PagureAPIClient):
log = logging.getLogger("zuul.test.FakePagureAPIClient")
def __init__(self, baseurl, api_token, project,
token_exp_date=None, pull_requests_db={}):
pull_requests_db={}):
super(FakePagureAPIClient, self).__init__(
baseurl, api_token, project, token_exp_date)
baseurl, api_token, project)
self.session = None
self.pull_requests = pull_requests_db
self.return_post_error = None
@ -1427,23 +1427,23 @@ class FakePagureConnection(pagureconnection.PagureConnection):
self.reports = []
self.rpcclient = rpcclient
self.cloneurl = self.upstream_root
self.connectors = {}
def _refresh_project_connectors(self, project):
connector = self.connectors.setdefault(
project, {'api_client': None, 'webhook_token': None})
api_token_exp_date = int(time.time()) + 60 * 24 * 3600
connector['api_client'] = FakePagureAPIClient(
self.baseurl, "fake_api_token-%s" % project, project,
token_exp_date=api_token_exp_date,
def get_project_api_client(self, project):
return FakePagureAPIClient(
self.baseurl, None, project,
pull_requests_db=self.pull_requests)
connector['webhook_token'] = "fake_webhook_token-%s" % project
return connector
def emitEvent(self, event, use_zuulweb=False, project=None):
def get_project_webhook_token(self, project):
return 'fake_webhook_token-%s' % project
def emitEvent(self, event, use_zuulweb=False, project=None,
wrong_token=False):
name, payload = event
secret = 'fake_webhook_token-%s' % project
if use_zuulweb:
if not wrong_token:
secret = 'fake_webhook_token-%s' % project
else:
secret = ''
payload = json.dumps(payload).encode('utf-8')
signature, _ = pagureconnection._sign_request(payload, secret)
headers = {'x-pagure-signature': signature,

19
tests/fixtures/zuul-pagure-driver-whitelist.conf vendored Normal file
View File

@ -0,0 +1,19 @@
[gearman]
server=127.0.0.1
[web]
status_url=http://zuul.example.com/status/#{change.number},{change.patchset}
[merger]
git_dir=/tmp/zuul-test/git
git_user_email=zuul@example.com
git_user_name=zuul
[executor]
git_dir=/tmp/zuul-test/executor-git
[connection pagure]
driver=pagure
server=pagure
api_token=0000000000000000000000000000000000000000
source_whitelist=::ffff:127.0.0.1

69
tests/unit/test_pagure_driver.py
View File

@ -992,7 +992,6 @@ class TestPagureWebhook(ZuulTestCase):
def setUp(self):
super(TestPagureWebhook, self).setUp()
# Start the web server
self.web = self.useFixture(
ZuulWebFixture(self.gearman_server.port,
@ -1010,14 +1009,18 @@ class TestPagureWebhook(ZuulTestCase):
self.fake_pagure.setZuulWebPort(port)
def tearDown(self):
super(TestPagureWebhook, self).tearDown()
@simple_layout('layouts/basic-pagure.yaml', driver='pagure')
def test_webhook(self):
A = self.fake_pagure.openFakePullRequest(
'org/project', 'master', 'A')
self.fake_pagure.emitEvent(A.getPullRequestOpenedEvent(),
use_zuulweb=True,
project='org/project',
wrong_token=True)
self.waitUntilSettled()
self.assertEqual(len(self.history), 0)
self.fake_pagure.emitEvent(A.getPullRequestOpenedEvent(),
use_zuulweb=True,
project='org/project')
@ -1029,38 +1032,40 @@ class TestPagureWebhook(ZuulTestCase):
self.getJobFromHistory('project-test2').result)
class TestPagureProjectConnector(ZuulTestCase):
config_file = 'zuul-pagure-driver.conf'
class TestPagureWebhookWhitelist(ZuulTestCase):
config_file = 'zuul-pagure-driver-whitelist.conf'
def setUp(self):
super(TestPagureWebhookWhitelist, self).setUp()
# Start the web server
self.web = self.useFixture(
ZuulWebFixture(self.gearman_server.port,
self.config, self.test_root))
host = '127.0.0.1'
# Wait until web server is started
while True:
port = self.web.port
try:
with socket.create_connection((host, port)):
break
except ConnectionRefusedError:
pass
self.fake_pagure.setZuulWebPort(port)
@simple_layout('layouts/basic-pagure.yaml', driver='pagure')
def test_connectors(self):
project_api_token_exp_date = self.fake_pagure.connectors[
'org/project']['api_client'].token_exp_date
def test_webhook_whitelist(self):
A = self.fake_pagure.openFakePullRequest(
'org/project', 'master', 'A')
self.fake_pagure.emitEvent(A.getPullRequestOpenedEvent())
self.fake_pagure.emitEvent(A.getPullRequestOpenedEvent(),
use_zuulweb=True,
project='org/project',
wrong_token=True)
self.waitUntilSettled()
self.assertEqual(
project_api_token_exp_date,
self.fake_pagure.connectors[
'org/project']['api_client'].token_exp_date)
# Now force a POST error with EINVALIDTOK code and check
# The connector has been refreshed
self.fake_pagure.connectors[
'org/project']['api_client'].return_post_error = {
'error': 'Invalid or expired token',
'error_code': 'EINVALIDTOK'
}
self.fake_pagure.emitEvent(A.getPullRequestUpdatedEvent())
self.waitUntilSettled()
# Expiry date changed meaning the token has been refreshed
self.assertNotEqual(
project_api_token_exp_date,
self.fake_pagure.connectors[
'org/project']['api_client'].token_exp_date)
self.assertEqual('SUCCESS',
self.getJobFromHistory('project-test1').result)
self.assertEqual('SUCCESS',
self.getJobFromHistory('project-test2').result)

187
zuul/driver/pagure/pagureconnection.py
View File

@ -1,4 +1,4 @@
# Copyright 2018 Red Hat, Inc.
# Copyright 2018, 2019 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
@ -38,34 +38,44 @@ from zuul.driver.pagure.paguremodel import PagureTriggerEvent, PullRequest
# Minimal Pagure version supported 5.3.0
#
# Pagure is similar to Github as it handles Pullrequest where PR is a branch
# Pagure is similar to Github as it handles PullRequest where PR is a branch
# composed of one or more commits. A PR can be commented, evaluated, updated,
# CI flagged, and merged. A PR can be flagged (success/failure/pending) and
# this driver use that capability. Code review (evaluation) is done via
# comments that contains a :thumbsup: or :thumbsdown:. Pagure computes a
# score based on that and allow or not the merge of PR if the "minimal score to
# merge" is set in repository settings. This driver uses that setting and need
# to be set. This driver expects to receive repository events via webhooks and
# expects to verify payload signature. The driver connection needs an user's
# API key with the "Modify an existing project" access. This user needs to be
# added as admin against projects to be gated by Zuul.
# this driver uses that capability.
#
# PR approval can be driven by (evaluation). This is done via comments that
# contains a :thumbsup: or :thumbsdown:. Pagure computes a score based on
# that and allows or not the merge of PR if the "minimal score to merge" is
# set in repository settings.
#
# PR approval can be also driven via PR metadata flag.
#
# This driver expects to receive repository events via webhooks and
# do event validation based on the source IP address of the event.
#
# The driver connection needs an user's API token with
# - "Merge a pull-request"
# - "Flag a pull-request"
# - "Comment on a pull-request"
#
# On each project to be integrated with Zuul needs:
#
# The web hook target must be (in repository settings):
# - http://<zuul-web>/zuul/api/connection/<conn-name>/payload
#
# Repository settings (to be checked):
# - Always merge (Better to match internal merge strategy of Zuul)
# - Minimum score to merge pull-request
# - Minimum score to merge pull-request = 0 or -1
# - Notify on pull-request flag
# - Pull requests
# - Open metadata access to all (unchecked if approval)
#
# To define the connection in /etc/zuul/zuul.conf:
# [connection pagure.sftests.com]
# [connection pagure.io]
# driver=pagure
# server=pagure.sftests.com
# baseurl=https://pagure.sftests.com/pagure
# cloneurl=https://pagure.sftests.com/pagure/git
# server=pagure.io
# baseurl=https://pagure.io
# api_token=QX29SXAW96C2CTLUNA5JKEEU65INGWTO2B5NHBDBRMF67S7PYZWCS0L1AKHXXXXX
# source_whitelist=8.43.85.75
#
# Current Non blocking issues:
# - Pagure does not reset the score when a PR code is updated
@ -85,9 +95,6 @@ from zuul.driver.pagure.paguremodel import PagureTriggerEvent, PullRequest
# https://docs.pagure.org/pagure/usage/project_settings.html?highlight=score#activate-only-assignee-can-merge-pull-request
TOKEN_VALIDITY = 60 * 24 * 3600
def _sign_request(body, secret):
signature = hmac.new(
secret.encode('utf-8'), body, hashlib.sha1).hexdigest()
@ -400,33 +407,22 @@ class PagureAPIClient():
log = logging.getLogger("zuul.PagureAPIClient")
def __init__(
self, baseurl, api_token, project, token_exp_date=None):
self, baseurl, api_token, project):
self.session = requests.Session()
self.base_url = '%s/api/0/' % baseurl
self.api_token = api_token
self.project = project
self.headers = {'Authorization': 'token %s' % self.api_token}
self.token_exp_date = token_exp_date
def _manage_error(self, data, code, url, verb):
if code < 400:
return
else:
if data.get('error_code', '') == 'EINVALIDTOK':
# Reset the expiry date of the cached API client
# to force the driver to refresh connectors
self.token_exp_date = int(time.time())
raise PagureAPIClientException(
"Unable to %s on %s (code: %s) due to: %s" % (
verb, url, code, data
))
def is_expired(self):
if self.token_exp_date:
if int(time.time()) > (self.token_exp_date - 3600):
return True
return False
def get(self, url):
self.log.debug("Getting resource %s ..." % url)
ret = self.session.get(url, headers=self.headers)
@ -497,72 +493,13 @@ class PagureAPIClient():
self._manage_error(*resp)
return resp[0]
def create_project_api_token(self):
def get_webhook_token(self):
""" A project admin user's api token must be use with that endpoint
"""
param = {
"description": "zuul-token-%s" % int(time.time()),
"acls": [
"pull_request_merge", "pull_request_comment",
"pull_request_flag"]
}
path = '%s/token/new' % self.project
resp = self.post(self.base_url + path, param)
self._manage_error(*resp)
# {"token": {"description": "mytoken", "id": "IED2HC...4QIXS6WPZDTET"}}
return resp[0]['token']
def get_connectors(self):
""" A project admin user's api token must be use with that endpoint
"""
def get_token_epoch(token):
return int(token['description'].split('-')[-1])
path = '%s/connector' % self.project
resp = self.get(self.base_url + path)
if resp[1] >= 400:
# Admin API token is probably invalid or expired
self.log.error(
("Unable to get connectors for project %s probably due to "
"an invalid or expired admin API token: %s") % (
self.project, resp[0]))
# Allow to process but return empty project API and webhook token
# Web hook events for the related project will be denied and
# POST on the API will be denied as well.
return {"id": "", "created_at": int(time.time())}, ""
data = resp[0]
# {"connector": {
# "hook_token": "WCL92MLWMRPGKBQ5LI0LZCSIS4TRQMHR0Q",
# "api_tokens": [
# {
# "description": "zuul-token-123",
# "expired": false,
# "id": "X03J4DOJT7P3G4....3DNPPXN4G144BBIAJ"
# }
# ]
# }}
# Filter expired tokens
tokens = [
token for token in data['connector'].get('api_tokens', {})
if not token['expired']]
# Now following the pattern zuul-token-{epoch} find the last
# one created
api_token = None
for token in tokens:
if not token['description'].startswith('zuul-token-'):
continue
epoch = get_token_epoch(token)
if api_token:
if epoch > get_token_epoch(api_token):
api_token = token
else:
api_token = token
if not api_token:
# Let's create one
api_token = self.create_project_api_token()
api_token['created_at'] = get_token_epoch(api_token)
webhook_token = data['connector']['hook_token']
return api_token, webhook_token
self._manage_error(*resp)
return resp[0]['connector']['hook_token']
class PagureConnection(BaseConnection):
@ -580,12 +517,14 @@ class PagureConnection(BaseConnection):
self.canonical_hostname = self.connection_config.get(
'canonical_hostname', self.server)
self.git_ssh_key = self.connection_config.get('sshkey')
self.admin_api_token = self.connection_config.get('api_token')
self.api_token = self.connection_config.get('api_token')
self.baseurl = self.connection_config.get(
'baseurl', 'https://%s' % self.server).rstrip('/')
self.cloneurl = self.connection_config.get(
'cloneurl', self.baseurl).rstrip('/')
self.connectors = {}
self.source_whitelist = self.connection_config.get(
'source_whitelist', '').split(',')
self.webhook_tokens = {}
self.source = driver.getSource(self)
self.event_queue = queue.Queue()
self.metadata_notif = re.compile(
@ -623,45 +562,23 @@ class PagureConnection(BaseConnection):
def eventDone(self):
self.event_queue.task_done()
def _refresh_project_connectors(self, project):
pagure = PagureAPIClient(
self.baseurl, self.admin_api_token, project)
api_token, webhook_token = pagure.get_connectors()
connector = self.connectors.setdefault(
project, {'api_client': None, 'webhook_token': None})
api_token_exp_date = api_token['created_at'] + TOKEN_VALIDITY
connector['api_client'] = PagureAPIClient(
self.baseurl, api_token['id'], project,
token_exp_date=api_token_exp_date)
connector['webhook_token'] = webhook_token
return connector
def get_project_api_client(self, project):
self.log.debug("Building project %s api_client" % project)
return PagureAPIClient(self.baseurl, self.api_token, project)
def get_project_webhook_token(self, project):
token = self.connectors.get(
project, {}).get('webhook_token', None)
token = self.webhook_tokens.get(project)
if token:
self.log.debug(
"Fetching project %s webhook_token from cache" % project)
"Fetching project %s webhook token from cache" % project)
return token
else:
pagure = self.get_project_api_client(project)
token = pagure.get_webhook_token()
self.webhook_tokens[project] = token
self.log.debug(
"Fetching project %s webhook_token from API" % project)
return self._refresh_project_connectors(project)['webhook_token']
def get_project_api_client(self, project):
api_client = self.connectors.get(
project, {}).get('api_client', None)
if api_client:
if not api_client.is_expired():
self.log.debug(
"Fetching project %s api_client from cache" % project)
return api_client
else:
self.log.debug(
"Project %s api token is expired (expiration date %s)" % (
project, api_client.token_exp_date))
self.log.debug("Building project %s api_client" % project)
return self._refresh_project_connectors(project)['api_client']
"Fetching project %s webhook token from API" % project)
return token
def maintainCache(self, relevant):
remove = set()
@ -928,6 +845,12 @@ class PagureWebController(BaseWebController):
self.connection = connection
self.zuul_web = zuul_web
def _source_whitelisted(self, remote_ip, forwarded_ip):
if remote_ip and remote_ip in self.connection.source_whitelist:
return True
if forwarded_ip and forwarded_ip in self.connection.source_whitelist:
return True
def _validate_signature(self, body, headers):
try:
request_signature = headers['x-pagure-signature']
@ -961,9 +884,15 @@ class PagureWebController(BaseWebController):
for key, value in cherrypy.request.headers.items():
headers[key.lower()] = value
body = cherrypy.request.body.read()
payload = self._validate_signature(body, headers)
json_payload = json.loads(payload.decode('utf-8'))
if not self._source_whitelisted(
getattr(cherrypy.request.remote, 'ip'),
headers.get('x-forwarded-for')):
self._validate_signature(body, headers)
else:
self.log.info(
"Payload origin IP address whitelisted. Skip verify")
json_payload = json.loads(body.decode('utf-8'))
job = self.zuul_web.rpc.submitJob(
'pagure:%s:payload' % self.connection.connection_name,
{'payload': json_payload})