From 83f805eb72f5b34d54a8e9499ff595502410de09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Piliszek?= Date: Thu, 8 Sep 2022 13:59:44 +0000 Subject: [PATCH] Improve ExternalServiceFilter Apply my comments from the original patch [1] and add unit tests. [1] https://review.opendev.org/c/openstack/blazar/+/781917 Change-Id: I116aad6677423aedb5dda48f06b36afc32819ba8 --- blazar/enforcement/exceptions.py | 5 + blazar/enforcement/filters/base_filter.py | 7 +- .../filters/external_service_filter.py | 179 +++++++------ .../filters/test_external_service_filter.py | 252 ++++++++++++++++++ doc/source/admin/usage-enforcement.rst | 50 +++- 5 files changed, 398 insertions(+), 95 deletions(-) create mode 100644 blazar/tests/enforcement/filters/test_external_service_filter.py diff --git a/blazar/enforcement/exceptions.py b/blazar/enforcement/exceptions.py index b30f355f..d99eaff3 100644 --- a/blazar/enforcement/exceptions.py +++ b/blazar/enforcement/exceptions.py @@ -21,3 +21,8 @@ class MaxLeaseDurationException(exceptions.NotAuthorized): msg_fmt = _('Lease duration of %(lease_duration)s seconds must be less ' 'than or equal to the maximum lease duration of ' '%(max_duration)s seconds.') + + +class ExternalServiceFilterException(exceptions.BlazarException): + code = 400 + msg_fmt = _('%(message)s') diff --git a/blazar/enforcement/filters/base_filter.py b/blazar/enforcement/filters/base_filter.py index 4fd3d5f2..ed14e994 100644 --- a/blazar/enforcement/filters/base_filter.py +++ b/blazar/enforcement/filters/base_filter.py @@ -22,9 +22,12 @@ class BaseFilter(metaclass=abc.ABCMeta): def __init__(self, conf=None): self.conf = conf + self.register_opts(conf) - for opt in self.enforcement_opts: - self.conf.register_opt(opt, 'enforcement') + @classmethod + def register_opts(cls, conf): + for opt in cls.enforcement_opts: + conf.register_opt(opt, 'enforcement') def __getattr__(self, name): func = getattr(self.conf.enforcement, name) diff --git a/blazar/enforcement/filters/external_service_filter.py b/blazar/enforcement/filters/external_service_filter.py index 687231ba..1b585039 100644 --- a/blazar/enforcement/filters/external_service_filter.py +++ b/blazar/enforcement/filters/external_service_filter.py @@ -13,14 +13,16 @@ # See the License for the specific language governing permissions and # limitations under the License. -import datetime +from datetime import datetime import json import requests +from urllib.parse import urljoin +from urllib.parse import urlparse +from blazar.enforcement.exceptions import ExternalServiceFilterException from blazar.enforcement.filters import base_filter -from blazar import exceptions +from blazar.exceptions import BlazarException from blazar.i18n import _ -from blazar.utils.openstack.keystone import BlazarKeystoneClient from oslo_config import cfg from oslo_log import log as logging @@ -28,29 +30,18 @@ from oslo_log import log as logging LOG = logging.getLogger(__name__) -class DateTimeEncoder(json.JSONEncoder): +class ISODateTimeEncoder(json.JSONEncoder): def default(self, o): - if isinstance(o, datetime.datetime): - return str(o) + if isinstance(o, datetime): + return o.isoformat() return json.JSONEncoder.default(self, o) -class ExternalServiceUnsupportedHTTPResponse(exceptions.BlazarException): - code = 400 - msg_fmt = _('External service enforcement filter returned a %(status)s ' - 'HTTP response. Only 204 and 403 responses are supported.') +GENERIC_DENY_MSG = 'External service enforcement filter denied the request.' -class ExternalServiceUnsupportedDeniedResponse(exceptions.BlazarException): - code = 400 - msg_fmt = _('External service enforcement filter returned a 403 HTTP ' - 'response %(response)s without a valid JSON dictionary ' - 'containing a "message" key.') - - -class ExternalServiceFilterException(exceptions.NotAuthorized): - code = 400 +class ExternalServiceMisconfigured(BlazarException): msg_fmt = _('%(message)s') @@ -58,21 +49,21 @@ class ExternalServiceFilter(base_filter.BaseFilter): enforcement_opts = [ cfg.StrOpt( - 'external_service_endpoint', + 'external_service_base_endpoint', default=None, help='The URL of the external service API.'), cfg.StrOpt( - 'external_service_check_create', + 'external_service_check_create_endpoint', default=None, - help='Overwrite check-create endpoint with absolute URL.'), + help='Overrides check-create endpoint with another URL.'), cfg.StrOpt( - 'external_service_check_update', + 'external_service_check_update_endpoint', default=None, - help='Overwrite check-update endpoint with absolute URL.'), + help='Overrides check-update endpoint with another URL.'), cfg.StrOpt( - 'external_service_on_end', + 'external_service_on_end_endpoint', default=None, - help='Overwrite on-end endpoint with absolute URL.'), + help='Overrides on-end endpoint with another URL.'), cfg.StrOpt( 'external_service_token', default="", @@ -82,75 +73,99 @@ class ExternalServiceFilter(base_filter.BaseFilter): def __init__(self, conf=None): super(ExternalServiceFilter, self).__init__(conf=conf) - def get_headers(self): + self._validate_url(conf.enforcement.external_service_base_endpoint) + self.base_endpoint = conf.enforcement.external_service_base_endpoint + + self.check_create_endpoint = self._construct_url( + "check-create", + conf.enforcement.external_service_check_create_endpoint) + self.check_update_endpoint = self._construct_url( + "check-update", + conf.enforcement.external_service_check_update_endpoint) + self.on_end_endpoint = self._construct_url( + "on-end", + conf.enforcement.external_service_on_end_endpoint) + + endpoints = ( + self.check_create_endpoint, + self.check_update_endpoint, + self.on_end_endpoint, + ) + + if all(x is None for x in endpoints): + raise ExternalServiceMisconfigured( + message=_("ExternalService has no endpoints set.")) + + self.token = conf.enforcement.external_service_token + + @staticmethod + def _validate_url(url): + if url is None: + return + parsed_url = urlparse(url) + if parsed_url.scheme not in ("http", "https"): + raise ExternalServiceMisconfigured( + message=_("ExternalService URL scheme must be http(s): " + "%s") % url) + if parsed_url.netloc == '': + raise ExternalServiceMisconfigured( + message=_("ExternalService URL must have netloc: " + "%s") % url) + + def _construct_url(self, method, replacement_url): + if replacement_url is None: + if self.base_endpoint is None: + return None + return urljoin(self.base_endpoint, method) + + self._validate_url(replacement_url) + return replacement_url + + def _get_headers(self): headers = {'Content-Type': 'application/json'} - if self.external_service_token: - headers['X-Auth-Token'] = (self.external_service_token) - else: - client = BlazarKeystoneClient() - headers['X-Auth-Token'] = client.session.get_token() + if self.token: + headers['X-Auth-Token'] = self.token return headers - def _get_absolute_url(self, path): - url = self.external_service_endpoint + def _post(self, url, body): + body = json.dumps(body, cls=ISODateTimeEncoder) + res = requests.post(url, headers=self._get_headers(), data=body) - if url[-1] == '/': - url += path[1:] - else: - url += path - - return url - - def post(self, url, body): - body = json.dumps(body, cls=DateTimeEncoder) - req = requests.post(url, headers=self.get_headers(), data=body) - - if req.status_code == 204: + if res.status_code == 204: return True - elif req.status_code == 403: + elif res.status_code == 403: try: - message = req.json()['message'] + message = res.json()['message'] except (requests.JSONDecodeError, KeyError): - raise ExternalServiceUnsupportedDeniedResponse( - response=req.content) - - raise ExternalServiceFilterException(message=message) + # NOTE(yoctozepto): It is more secure not to send the actual + # response to the end user as it may leak something. + # Instead, we log it for debugging. + LOG.debug("The External Service API returned a malformed " + "response (403): %s", res.content) + message = GENERIC_DENY_MSG else: - raise ExternalServiceUnsupportedHTTPResponse( - status=req.status_code) + # NOTE(yoctozepto): It is more secure not to send the actual + # response to the end user as it may leak something. + # Instead, we log it for debugging. + LOG.debug("The External Service API returned a malformed " + "response (%d): %s", res.status_code, res.content) + message = GENERIC_DENY_MSG + raise ExternalServiceFilterException(message=message) def check_create(self, context, lease_values): - body = dict(context=context, lease=lease_values) - if self.external_service_check_create: - self.post(self.external_service_check_create, body) - return - - if self.external_service_endpoint: - path = '/check-create' - self.post(self._get_absolute_url(path), body) - return + if self.check_create_endpoint: + self._post(self.check_create_endpoint, dict( + context=context, lease=lease_values)) def check_update(self, context, current_lease_values, new_lease_values): - body = dict(context=context, current_lease=current_lease_values, - lease=new_lease_values) - if self.external_service_check_update: - self.post(self.external_service_check_update, body) - return - - if self.external_service_endpoint: - path = '/check-update' - self.post(self._get_absolute_url(path), body) - return + if self.check_update_endpoint: + self._post(self.check_update_endpoint, dict( + context=context, current_lease=current_lease_values, + lease=new_lease_values)) def on_end(self, context, lease_values): - body = dict(context=context, lease=lease_values) - if self.external_service_on_end: - self.post(self.external_service_on_end, body) - return - - if self.external_service_endpoint: - path = '/on-end' - self.post(self._get_absolute_url(path), body) - return + if self.on_end_endpoint: + self._post(self.on_end_endpoint, dict( + context=context, lease=lease_values)) diff --git a/blazar/tests/enforcement/filters/test_external_service_filter.py b/blazar/tests/enforcement/filters/test_external_service_filter.py new file mode 100644 index 00000000..9aee96a6 --- /dev/null +++ b/blazar/tests/enforcement/filters/test_external_service_filter.py @@ -0,0 +1,252 @@ +# Copyright (c) 2022 Radosław Piliszek +# +# 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 a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import datetime +import json +from unittest import mock + +from blazar.enforcement.exceptions import ExternalServiceFilterException +from blazar.enforcement.filters import external_service_filter +from blazar.tests import TestCase + +from oslo_config.cfg import CONF + + +class FakeResponse204(): + status_code = 204 + + +class FakeResponse403Empty(): + status_code = 403 + content = "irrelevant_but_logged" + + @staticmethod + def json(): + return {} + + +class FakeJSONDecodeError(Exception): + pass + + +class FakeResponse403InvalidJSON(): + status_code = 403 + content = "NOT_JSON" + + @staticmethod + def json(): + raise FakeJSONDecodeError() + + +class FakeResponse403WithMessage(): + status_code = 403 + content = "irrelevant" + + @staticmethod + def json(): + return {"message": "Hello!"} + + +class FakeResponse500(): + status_code = 500 + content = "ALL_YOUR_BUGS_BELONG_TO_US" + + +class ISODateTimeEncoderTestCase(TestCase): + + def test_json_date(self): + dt = datetime.datetime(2022, 9, 8, 13, 31, 44, 12345) + obj = {"datetime": dt} + x = json.dumps(obj, cls=external_service_filter.ISODateTimeEncoder) + self.assertEqual('{"datetime": "2022-09-08T13:31:44.012345"}', x) + + def test_json_with_tz(self): + tz = datetime.timezone(datetime.timedelta(hours=2)) + dt = datetime.datetime(2022, 9, 8, 13, 31, 44, 12345, tz) + obj = {"datetime": dt} + x = json.dumps(obj, cls=external_service_filter.ISODateTimeEncoder) + self.assertEqual('{"datetime": "2022-09-08T13:31:44.012345+02:00"}', x) + + +class ConfiguringExternalServiceFilterTestCase(TestCase): + def setUp(self): + super().setUp() + + external_service_filter.ExternalServiceFilter.register_opts(CONF) + + def test_basic_misconfiguration(self): + self.assertRaises(external_service_filter.ExternalServiceMisconfigured, + external_service_filter.ExternalServiceFilter, CONF) + + def test_bad_url(self): + CONF.set_override( + 'external_service_base_endpoint', 'this_url_cOuLDnOtBeWoRsE', + group='enforcement') + self.addCleanup(CONF.clear_override, 'external_service_base_endpoint', + group='enforcement') + + self.assertRaises(external_service_filter.ExternalServiceMisconfigured, + external_service_filter.ExternalServiceFilter, CONF) + + def test_check_create_endpoint_is_enough(self): + CONF.set_override( + 'external_service_check_create_endpoint', 'http://localhost', + group='enforcement') + self.addCleanup(CONF.clear_override, + 'external_service_check_create_endpoint', + group='enforcement') + + external_service_filter.ExternalServiceFilter(CONF) + + def test_check_updaye_endpoint_is_enough(self): + CONF.set_override( + 'external_service_check_update_endpoint', 'http://localhost', + group='enforcement') + self.addCleanup(CONF.clear_override, + 'external_service_check_update_endpoint', + group='enforcement') + + external_service_filter.ExternalServiceFilter(CONF) + + def test_on_end_endpoint_is_enough(self): + CONF.set_override( + 'external_service_on_end_endpoint', 'http://localhost', + group='enforcement') + self.addCleanup(CONF.clear_override, + 'external_service_on_end_endpoint', + group='enforcement') + + external_service_filter.ExternalServiceFilter(CONF) + + +class ExternalServiceFilterTestCase(TestCase): + def setUp(self): + super().setUp() + + external_service_filter.ExternalServiceFilter.register_opts(CONF) + + CONF.set_override( + 'external_service_base_endpoint', 'http://localhost', + group='enforcement') + self.addCleanup(CONF.clear_override, 'external_service_base_endpoint', + group='enforcement') + + self.filter = external_service_filter.ExternalServiceFilter(CONF) + + self.ctx = { + "is_context": True + } + + self.lease = { + "is_lease": True + } + + self.old_lease = { + "is_old_lease": True + } + + @mock.patch("requests.post") + def test_check_create_allowed(self, post_mock): + post_mock.return_value = FakeResponse204() + self.filter.check_create(self.ctx, self.lease) + post_mock.assert_called_with( + "http://localhost/check-create", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_check_create_denied(self, post_mock): + post_mock.return_value = FakeResponse403WithMessage() + self.assertRaises(ExternalServiceFilterException, + self.filter.check_create, + self.ctx, self.lease) + post_mock.assert_called_with( + "http://localhost/check-create", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_check_create_failed(self, post_mock): + post_mock.return_value = FakeResponse403Empty() + self.assertRaises(ExternalServiceFilterException, + self.filter.check_create, + self.ctx, self.lease) + post_mock.assert_called_with( + "http://localhost/check-create", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_check_update_allowed(self, post_mock): + post_mock.return_value = FakeResponse204() + self.filter.check_update(self.ctx, self.old_lease, self.lease) + post_mock.assert_called_with( + "http://localhost/check-update", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"current_lease": {"is_old_lease": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_check_update_denied(self, post_mock): + post_mock.return_value = FakeResponse403WithMessage() + self.assertRaises(ExternalServiceFilterException, + self.filter.check_update, + self.ctx, self.old_lease, self.lease) + post_mock.assert_called_with( + "http://localhost/check-update", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"current_lease": {"is_old_lease": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + @mock.patch("requests.JSONDecodeError", FakeJSONDecodeError) + def test_check_update_failed(self, post_mock): + post_mock.return_value = FakeResponse403InvalidJSON() + self.assertRaises(ExternalServiceFilterException, + self.filter.check_update, + self.ctx, self.old_lease, self.lease) + post_mock.assert_called_with( + "http://localhost/check-update", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"current_lease": {"is_old_lease": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_on_end_success(self, post_mock): + post_mock.return_value = FakeResponse204() + self.filter.on_end(self.ctx, self.lease) + post_mock.assert_called_with( + "http://localhost/on-end", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"lease": {"is_lease": true}}') + + @mock.patch("requests.post") + def test_on_end_failure(self, post_mock): + post_mock.return_value = FakeResponse500() + self.assertRaises(ExternalServiceFilterException, + self.filter.on_end, + self.ctx, self.lease) + post_mock.assert_called_with( + "http://localhost/on-end", + headers={'Content-Type': 'application/json'}, + data='{"context": {"is_context": true}, ' + '"lease": {"is_lease": true}}') diff --git a/doc/source/admin/usage-enforcement.rst b/doc/source/admin/usage-enforcement.rst index 449decbd..b37d940b 100644 --- a/doc/source/admin/usage-enforcement.rst +++ b/doc/source/admin/usage-enforcement.rst @@ -37,6 +37,11 @@ as follows: .. +Do note that filter config options follow filter names - the prefix is always +the snake case of the filter name (``MaxLeaseDurationFilter`` becomes +``max_lease_duration``; in this case it is special that there is nothing +beyond the prefix but there is also ``max_lease_duration_exempt_project_ids``). + MaxLeaseDurationFilter ---------------------- @@ -55,17 +60,32 @@ ExternalServiceFilter --------------------- This filter delegates the decision for each API to an external HTTP service. -The service must use token-based authentication and implement the following -endpoints for POST method: +The service must use token-based authentication, accepting (or ignoring) +the static token sent by Blazar in the ``X-Auth-Token`` header. +The following endpoints should be implemented: -* ``POST /v1/check-create`` -* ``POST /v1/check-update`` -* ``POST /v1/on-end`` +* ``POST /check-create`` +* ``POST /check-update`` +* ``POST /on-end`` + +The exact URLs can be overridden and not all have to be used (although +we imagine a proper implementation requires at least both checks unless +lease updates are disabled in the first place). The external service should return ``204 No Content`` if the parameters meet -defined criteria and ``403 Forbidden`` if not. +defined criteria and ``403 Forbidden`` if not. The service may send a JSON +body response with the ``403 Forbidden`` reply, including the rejection +reasoning in the field named ``message`` as in: -Example format of data the external service will receive in a request body: +.. sourcecode:: json + + { + "message": "You shall not pass!" + } + +An example of data the external service will receive in a request body (do note +all dates and times are encoded as strings following the ISO8601 standard that +is expected in JSON to represent dates and times): * Request example: @@ -79,8 +99,8 @@ Example format of data the external service will receive in a request body: "region_name": "RegionOne" }, "current_lease": { - "start_date": "2020-05-13 00:00", - "end_time": "2020-05-14 23:59", + "start_date": "2020-05-13T00:00:00.012345+02:00", + "end_time": "2020-05-14T23:59:00.012345+02:00", "reservations": [ { "resource_type": "physical:host", @@ -101,8 +121,8 @@ Example format of data the external service will receive in a request body: ] }, "lease": { - "start_date": "2020-05-13 00:00", - "end_time": "2020-05-14 23:59", + "start_date": "2020-05-13T00:00:00.012345+02:00", + "end_time": "2020-05-14T23:59:00.012345+02:00", "reservations": [ { "resource_type": "physical:host", @@ -130,3 +150,11 @@ Example format of data the external service will receive in a request body: ] } } + +The ``current_lease`` field is present only in ``check-update`` requests and +describes the existing lease. In both checks the ``lease`` field describes +the new lease. In ``on-end``, the ``lease`` field describes the lease that +has just ended. + +There is no guarantee on the delivery of the ``on-end`` event and it should be +considered an optimisation rather than a reliable mechanism.