From 7ff150ced70d0db565378683909e22dfac5383e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20=27sil2100=27=20Zemczak?= Date: Wed, 9 Jun 2021 17:13:56 +0200 Subject: [PATCH] Switch from swiftclient for private PPAs to using HTTP with X-Auth-Token instead. This way there's less secrets that need to be shared and less new code to introduce. We also modified the test tooling to be able to check for authentication tokens in the queries. --- britney.conf | 8 +- britney.conf.template | 8 +- britney2/policies/autopkgtest.py | 140 ++++++++++--------------------- tests/__init__.py | 8 +- tests/mock_swift.py | 34 ++++---- tests/mock_swiftclient.py | 70 ---------------- tests/test_autopkgtest.py | 18 ++-- tests/test_policy.py | 4 +- 8 files changed, 76 insertions(+), 214 deletions(-) delete mode 100644 tests/mock_swiftclient.py diff --git a/britney.conf b/britney.conf index ee4fc89..5701701 100644 --- a/britney.conf +++ b/britney.conf @@ -102,12 +102,10 @@ ADT_SHARED_RESULTS_CACHE = # Swift base URL with the results (must be publicly readable and browsable) # or file location if results are pre-fetched ADT_SWIFT_URL = https://autopkgtest.ubuntu.com/results/ -# Swift identity with read access to the private result container +# Swift authentication user and token for access to private containers # (this is required whenever a private PPA is used for testing) -ADT_SWIFT_USER = -ADT_SWIFT_PASS = -ADT_SWIFT_AUTH_URL = -ADT_SWIFT_TENANT = +ADT_SWIFT_USER = +ADT_SWIFT_TOKEN = # List of launchpad users/teams that should have read access to any private # result logs ADT_PRIVATE_SHARED = diff --git a/britney.conf.template b/britney.conf.template index 7c21e5f..4a10ca2 100644 --- a/britney.conf.template +++ b/britney.conf.template @@ -124,12 +124,10 @@ ADT_SHARED_RESULTS_CACHE = # or file location if results are pre-fetched #ADT_SWIFT_URL = https://example.com/some/url ADT_SWIFT_URL = file:///path/to/britney/state/debci.json -# Swift identity with read access to the private result container +# Swift authentication user and token for access to private containers # (this is required whenever a private PPA is used for testing) -ADT_SWIFT_USER = -ADT_SWIFT_PASS = -ADT_SWIFT_AUTH_URL = -ADT_SWIFT_TENANT = +ADT_SWIFT_USER = +ADT_SWIFT_TOKEN = # List of launchpad users/teams that should have read access to any private # result logs ADT_PRIVATE_SHARED = diff --git a/britney2/policies/autopkgtest.py b/britney2/policies/autopkgtest.py index bbfc2f3..e34684f 100644 --- a/britney2/policies/autopkgtest.py +++ b/britney2/policies/autopkgtest.py @@ -32,7 +32,7 @@ import sys import time import urllib.parse from urllib.error import HTTPError -from urllib.request import urlopen +from urllib.request import Request, urlopen import apt_pkg @@ -208,11 +208,9 @@ class AutopkgtestPolicy(BasePolicy): # log into swift in case we need to fetch some private results # this is optional - if there are no credentials present, results will # be fetched without authentication - if self.options.adt_swift_user: - if (not self.options.adt_swift_pass or - not self.options.adt_swift_auth_url or - not self.options.adt_swift_tenant): - raise RuntimeError('Incomplete swift credentials given') + if self.options.adt_swift_token: + if self.options.adt_swift_url.startswith('file://'): + raise RuntimeError('Token authentication requires swift remote storage') # once swift credentials are given, the results will be published # to a private container @@ -225,28 +223,10 @@ class AutopkgtestPolicy(BasePolicy): # TODO: write a test for this if '@' in ppa and not re.match(r'^.+:.+@.+:.+$', ppa): raise RuntimeError('Private PPA %s not following required format (user:token@team/name:fingerprint)', ppa) - - import swiftclient - - if '/v2.0' in self.options.adt_swift_auth_url: - auth_version = '2.0' - else: - auth_version = '1' - - self.logger.info('Creating an authenticated swift connection for user %s', self.options.adt_swift_user) - self.swift_conn = swiftclient.Connection( - authurl=self.options.adt_swift_auth_url, - user=self.options.adt_swift_user, - key=self.options.adt_swift_pass, - tenant_name=self.options.adt_swift_tenant, - auth_version=auth_version - ) else: if any('@' in ppa for ppa in self.options.adt_ppas): raise RuntimeError('Private PPA configured but no swift credentials given') - self.swift_conn = None - # read in the new results if self.options.adt_swift_url.startswith('file://'): debci_file = self.options.adt_swift_url[7:] @@ -855,10 +835,12 @@ class AutopkgtestPolicy(BasePolicy): latest_run_for_package._cache = collections.defaultdict(dict) - def download_retry(self, url): + def download_retry(self, url, token=None): + headers = {'X-Auth-Token': token} if token else {} + request = Request(url, headers=headers) for retry in range(5): try: - req = urlopen(url, timeout=30) + req = urlopen(request, timeout=30) code = req.getcode() if 200 <= code < 300: return req @@ -903,54 +885,32 @@ class AutopkgtestPolicy(BasePolicy): query['marker'] = query['prefix'] + latest_run_id # request new results from swift - if self.swift_conn: - # when we have an authenticated swift connection, use that to - # fetch the result_path list as we might be fetching from an - # otherwise unaccessible container - from swiftclient.exceptions import ClientException - - try: - _, result_paths = self.swift_conn.get_container( - self.swift_container, - query_string=urllib.parse.urlencode(query)) - except ClientException as e: - # 401 "Unauthorized" is swift's way of saying "container does not exist" - if e.http_status == '401' or e.http_status == '404': - self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', self.swift_container) - return - # Other status codes are usually a transient - # network/infrastructure failure. Ignoring this can lead to - # re-requesting tests which we already have results for, so - # fail hard on this and let the next run retry. - self.logger.error('Failure to fetch swift results from %s: %s', self.swift_container, str(e)) - sys.exit(1) - else: - url = os.path.join(swift_url, self.swift_container) - url += '?' + urllib.parse.urlencode(query) - f = None - try: - f = self.download_retry(url) - if f.getcode() == 200: - result_paths = f.read().decode().strip().splitlines() - elif f.getcode() == 204: # No content - result_paths = [] - else: - # we should not ever end up here as we expect a HTTPError in - # other cases; e. g. 3XX is something that tells us to adjust - # our URLS, so fail hard on those - raise NotImplementedError('fetch_swift_results(%s): cannot handle HTTP code %i' % - (url, f.getcode())) - except IOError as e: - # 401 "Unauthorized" is swift's way of saying "container does not exist" - if hasattr(e, 'code') and e.code == 401: - self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', url) - return - # same as above in the swift authenticated case - self.logger.error('Failure to fetch swift results from %s: %s', url, str(e)) - sys.exit(1) - finally: - if f is not None: - f.close() + url = os.path.join(swift_url, self.swift_container) + url += '?' + urllib.parse.urlencode(query) + f = None + try: + f = self.download_retry(url, self.options.adt_swift_token) + if f.getcode() == 200: + result_paths = f.read().decode().strip().splitlines() + elif f.getcode() == 204: # No content + result_paths = [] + else: + # we should not ever end up here as we expect a HTTPError in + # other cases; e. g. 3XX is something that tells us to adjust + # our URLS, so fail hard on those + raise NotImplementedError('fetch_swift_results(%s): cannot handle HTTP code %i' % + (url, f.getcode())) + except IOError as e: + # 401 "Unauthorized" is swift's way of saying "container does not exist" + if hasattr(e, 'code') and e.code == 401: + self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', url) + return + # same as above in the swift authenticated case + self.logger.error('Failure to fetch swift results from %s: %s', url, str(e)) + sys.exit(1) + finally: + if f is not None: + f.close() for p in result_paths: self.fetch_one_result( @@ -966,29 +926,13 @@ class AutopkgtestPolicy(BasePolicy): f = None try: - if self.swift_conn: - from swiftclient.exceptions import ClientException - - # We don't need any additional retry logic as swiftclient - # already performs retries (5 by default). - url = os.path.join(path, name) - try: - _, contents = self.swift_conn.get_object(container, url) - except ClientException as e: - self.logger.error('Failure to fetch %s from container %s: %s', - url, container, str(e)) - if e.http_status == '404': - return - sys.exit(1) - tar_bytes = io.BytesIO(contents) + url = os.path.join(swift_url, container, path, name) + f = self.download_retry(url, self.options.adt_swift_token) + if f.getcode() == 200: + tar_bytes = io.BytesIO(f.read()) else: - url = os.path.join(swift_url, container, path, name) - f = self.download_retry(url) - if f.getcode() == 200: - tar_bytes = io.BytesIO(f.read()) - else: - raise NotImplementedError('fetch_one_result(%s): cannot handle HTTP code %i' % - (url, f.getcode())) + raise NotImplementedError('fetch_one_result(%s): cannot handle HTTP code %i' % + (url, f.getcode())) except IOError as e: self.logger.error('Failure to fetch %s: %s', url, str(e)) # we tolerate "not found" (something went wrong on uploading the @@ -1116,7 +1060,7 @@ class AutopkgtestPolicy(BasePolicy): qname = 'debci-%s-%s' % (self.options.series, arch) params['submit-time'] = datetime.strftime(datetime.utcnow(), '%Y-%m-%d %H:%M:%S%z') - if self.swift_conn: + if self.options.adt_swift_user: params['swiftuser'] = self.options.adt_swift_user if self.options.adt_private_shared: params['readable-by'] = self.options.adt_private_shared @@ -1329,7 +1273,7 @@ class AutopkgtestPolicy(BasePolicy): 'log.gz') else: # Private runs have a different base url - results_url = self.options.adt_private_url if self.swift_conn else self.options.adt_swift_url + results_url = self.options.adt_private_url if self.options.adt_swift_user else self.options.adt_swift_url url = os.path.join(results_url, self.swift_container, self.options.series, diff --git a/tests/__init__.py b/tests/__init__.py index acbe475..7fab7b9 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -395,11 +395,9 @@ ADT_PPAS = ADT_SHARED_RESULTS_CACHE = ADT_SWIFT_URL = http://localhost:18085 -ADT_SWIFT_USER = -ADT_SWIFT_PASS = -ADT_SWIFT_AUTH_URL = -ADT_SWIFT_TENANT = -ADT_PRIVATE_SHARED = +ADT_SWIFT_USER = +ADT_SWIFT_TOKEN = +ADT_PRIVATE_SHARED = ADT_PRIVATE_URL = ADT_CI_URL = https://autopkgtest.ubuntu.com/ ADT_HUGE = 20 diff --git a/tests/mock_swift.py b/tests/mock_swift.py index 0fc1a4c..1d5c0db 100644 --- a/tests/mock_swift.py +++ b/tests/mock_swift.py @@ -32,6 +32,7 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): ''' # map container -> result.tar path -> (exitcode, testpkg-version[, testinfo]) results = {} + expected_token = 'SomeSecretToken' def do_GET(self): p = urlparse(self.path) @@ -43,7 +44,22 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): else: self.list_container(container, parse_qs(p.query)) + def check_if_authorized(self, container): + status = True + if container.startswith('private-'): + # private container, so we need authentication + token = self.headers.get('X-Auth-Token') + if not token: + self.send_error(401, 'Unauthorized') + status = False + elif self.expected_token != token: + self.send_error(403, 'Forbidden') + status = False + return status + def serve_file(self, container, path): + if not self.check_if_authorized(container): + return if os.path.basename(path) != 'result.tar': self.send_error(404, 'File not found (only result.tar supported)') return @@ -85,6 +101,8 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): self.wfile.write(tar.getvalue()) def list_container(self, container, query): + if not self.check_if_authorized(container): + return try: objs = set(['%s/result.tar' % r for r in self.results[container]]) except KeyError: @@ -128,15 +146,7 @@ class AutoPkgTestSwiftServer: ''' SwiftHTTPRequestHandler.results = results - def start(self, swiftclient=False): - if swiftclient: - # since we're running britney directly, the only way to reliably - # mock out the swiftclient module is to override it in the local - # path with the dummy version we created - src = os.path.join(TESTS_DIR, 'mock_swiftclient.py') - dst = os.path.join(PROJECT_DIR, 'swiftclient.py') - os.symlink(src, dst) - + def start(self): assert self.server_pid is None, 'already started' if self.log: self.log.close() @@ -160,12 +170,6 @@ class AutoPkgTestSwiftServer: sys.exit(0) def stop(self): - # in case we were 'mocking out' swiftclient, remove the symlink we - # created earlier during start() - swiftclient_mod = os.path.join(PROJECT_DIR, 'swiftclient.py') - if os.path.islink(swiftclient_mod): - os.unlink(swiftclient_mod) - assert self.server_pid, 'not running' os.kill(self.server_pid, 15) os.waitpid(self.server_pid, 0) diff --git a/tests/mock_swiftclient.py b/tests/mock_swiftclient.py deleted file mode 100644 index 5f419ac..0000000 --- a/tests/mock_swiftclient.py +++ /dev/null @@ -1,70 +0,0 @@ -# Mock the swiftclient Python library, the bare minimum for ADT purposes -# Author: Łukasz 'sil2100' Zemczak - -import os -import sys - -from urllib.request import urlopen - - -# We want to use this single Python module file to mock out the exception -# module as well. -sys.modules["swiftclient.exceptions"] = sys.modules[__name__] - - -class ClientException(Exception): - def __init__(self, msg, http_status=''): - super(ClientException, self).__init__(msg) - self.msg = msg - self.http_status = http_status - - -class Connection: - def __init__(self, authurl, user, key, tenant_name, auth_version): - self._mocked_swift = 'http://localhost:18085' - - def get_container(self, container, marker=None, limit=None, prefix=None, - delimiter=None, end_marker=None, path=None, - full_listing=False, headers=None, query_string=None): - url = os.path.join(self._mocked_swift, container) + '?' + query_string - req = None - try: - req = urlopen(url, timeout=30) - code = req.getcode() - if code == 200: - result_paths = req.read().decode().strip().splitlines() - elif code == 204: # No content - result_paths = [] - else: - raise ClientException('MockedError', http_status=str(code)) - except IOError as e: - # 401 "Unauthorized" is swift's way of saying "container does not exist" - # But here we just assume swiftclient handles this via the usual - # ClientException. - raise ClientException('MockedError', http_status=str(e.code) if hasattr(e, 'code') else '') - finally: - if req is not None: - req.close() - - return (None, result_paths) - - def get_object(self, container, obj): - url = os.path.join(self._mocked_swift, container, obj) - req = None - try: - req = urlopen(url, timeout=30) - code = req.getcode() - if code == 200: - contents = req.read() - else: - raise ClientException('MockedError', http_status=str(code)) - except IOError as e: - # 401 "Unauthorized" is swift's way of saying "container does not exist" - # But here we just assume swiftclient handles this via the usual - # ClientException. - raise ClientException('MockedError', http_status=str(e.code) if hasattr(e, 'code') else '') - finally: - if req is not None: - req.close() - - return (None, contents) diff --git a/tests/test_autopkgtest.py b/tests/test_autopkgtest.py index 281889a..74b4766 100644 --- a/tests/test_autopkgtest.py +++ b/tests/test_autopkgtest.py @@ -117,7 +117,7 @@ class TestAutopkgtestBase(TestBase): with open(email_path, 'w', encoding='utf-8') as email: email.write(json.dumps(self.email_cache)) - self.swift.start(swiftclient=True) + self.swift.start() (excuses_yaml, excuses_html, out) = self.run_britney() self.swift.stop() @@ -2556,12 +2556,8 @@ class AT(TestAutopkgtestBase): print('ADT_PPAS = first/ppa user:password@joe/foo:DEADBEEF') elif line.startswith('ADT_SWIFT_USER'): print('ADT_SWIFT_USER = user') - elif line.startswith('ADT_SWIFT_PASS'): - print('ADT_SWIFT_PASS = pass') - elif line.startswith('ADT_SWIFT_TENANT'): - print('ADT_SWIFT_TENANT = tenant') - elif line.startswith('ADT_SWIFT_AUTH_URL'): - print('ADT_SWIFT_AUTH_URL = http://127.0.0.1:5000/v2.0/') + elif line.startswith('ADT_SWIFT_TOKEN'): + print('ADT_SWIFT_TOKEN = SomeSecretToken') elif line.startswith('ADT_PRIVATE_SHARED'): print('ADT_PRIVATE_SHARED = user1 team2') elif line.startswith('ADT_PRIVATE_URL'): @@ -2628,12 +2624,8 @@ class AT(TestAutopkgtestBase): for line in fileinput.input(self.britney_conf, inplace=True): if line.startswith('ADT_SWIFT_USER'): print('ADT_SWIFT_USER = user') - elif line.startswith('ADT_SWIFT_PASS'): - print('ADT_SWIFT_PASS = pass') - elif line.startswith('ADT_SWIFT_TENANT'): - print('ADT_SWIFT_TENANT = tenant') - elif line.startswith('ADT_SWIFT_AUTH_URL'): - print('ADT_SWIFT_AUTH_URL = http://127.0.0.1:5000/v2.0/') + elif line.startswith('ADT_SWIFT_TOKEN'): + print('ADT_SWIFT_TOKEN = SomeSecretToken') elif line.startswith('ADT_PRIVATE_URL'): print('ADT_PRIVATE_URL = http://localhost:18085/private-results/') else: diff --git a/tests/test_policy.py b/tests/test_policy.py index c6600c0..d7c411a 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -44,9 +44,7 @@ def initialize_policy(test_name, policy_class, *args, **kwargs): adt_swift_url='file://' + debci_data, adt_ci_url='', adt_swift_user='', - adt_swift_pass='', - adt_swift_tenant='', - adt_swift_auth_url='', + adt_swift_token='', adt_private_shared=[], adt_private_url='', adt_success_bounty=3,