From 134c19faeb12ed81f13d84e83d96872d58843d72 Mon Sep 17 00:00:00 2001 From: He Jie Xu Date: Thu, 8 Dec 2016 19:25:58 +0800 Subject: [PATCH] Add query parameters white list for server list/detail This patch enables a white list for query parameter. The parameters, which are out of white list, will be ignored. The sort_key still accepts open value. The later patch will add white list to it. Co-Authored-By: Zhenyu Zheng partial implement of bp add-whitelist-for-server-list-filter-sort-parameters Change-Id: I7141eef6a1c85ec6d6e8ee170911572535652978 --- nova/api/openstack/compute/schemas/servers.py | 99 +++++++++++++++++++ nova/api/openstack/compute/servers.py | 29 +++--- nova/api/validation/__init__.py | 13 ++- nova/api/validation/validators.py | 11 +++ .../api/openstack/compute/test_serversV21.py | 25 +++-- nova/tests/unit/test_api_validation.py | 51 +++++++++- 6 files changed, 202 insertions(+), 26 deletions(-) diff --git a/nova/api/openstack/compute/schemas/servers.py b/nova/api/openstack/compute/schemas/servers.py index 7a9d13ef8f..6f1e834fbd 100644 --- a/nova/api/openstack/compute/schemas/servers.py +++ b/nova/api/openstack/compute/schemas/servers.py @@ -15,6 +15,7 @@ import copy from nova.api.validation import parameter_types +from nova.api.validation.parameter_types import multi_params base_create = { @@ -232,3 +233,101 @@ trigger_crash_dump = { 'required': ['trigger_crash_dump'], 'additionalProperties': False } + +# NOTE: We don't check actual values of queries on params +# which are defined as the following common_param. +common_param = multi_params({'type': 'string'}) +common_regex_param = multi_params({'type': 'string', 'format': 'regex'}) + +JOINED_TABLE_QUERY_PARAMS_SERVERS = { + 'block_device_mapping': common_param, + 'services': common_param, + 'metadata': common_param, + 'system_metadata': common_param, + 'info_cache': common_param, + 'security_groups': common_param, + 'pci_devices': common_param +} + +query_params_v21 = { + 'type': 'object', + 'properties': { + 'user_id': common_param, + 'project_id': common_param, + # The alias of project_id. It should be removed in the + # future with microversion bump. + 'tenant_id': common_param, + 'launch_index': common_param, + # The alias of image. It should be removed in the + # future with microversion bump. + 'image_ref': common_param, + 'image': common_param, + 'kernel_id': common_param, + 'ramdisk_id': common_param, + 'hostname': common_param, + 'key_name': common_param, + 'power_state': common_param, + 'vm_state': common_param, + 'task_state': common_param, + 'host': common_param, + 'node': common_param, + 'flavor': common_param, + 'reservation_id': common_param, + 'launched_at': common_param, + 'terminate_at': common_param, + 'availability_zone': common_param, + # NOTE(alex_xu): This is pattern matching, it didn't get any benefit + # from DB index. + 'name': common_regex_param, + # The alias of name. It should be removed in the future + # with microversion bump. + 'display_name': common_regex_param, + 'description': common_regex_param, + # The alias of description. It should be removed in the + # future with microversion bump. + 'display_description': common_regex_param, + 'locked_by': common_param, + 'uuid': common_param, + 'root_device_name': common_param, + 'config_drive': common_param, + 'accessIPv4': common_param, + 'accessIPv6': common_param, + 'auto_disk_config': common_param, + 'progress': common_param, + 'sort_key': common_param, + 'sort_dir': common_param, + 'all_tenants': common_param, + 'deleted': common_param, + 'status': common_param, + 'changes-since': multi_params({'type': 'string', + 'format': 'date-time'}), + # NOTE(alex_xu): The ip and ip6 are implemented in the python. + 'ip': common_regex_param, + 'ip6': common_regex_param, + 'created_at': common_param, + }, + # For backward-compatible additionalProperties is set to be True here. + # And we will either strip the extra params out or raise HTTP 400 + # according to the params' value in the later process. + 'additionalProperties': True, + # Prevent internal-attributes that are started with underscore from + # being striped out in schema validation, and raise HTTP 400 in API. + 'patternProperties': {"^_": common_param} +} + +# Update the joined-table fields to the list so it will not be +# stripped in later process, thus can be handled later in api +# to raise HTTP 400. +query_params_v21['properties'].update( + JOINED_TABLE_QUERY_PARAMS_SERVERS) + +query_params_v21['properties'].update( + parameter_types.pagination_parameters) + +query_params_v226 = copy.deepcopy(query_params_v21) +query_params_v226['properties'].update({ + 'tags': common_param, + 'tags-any': common_param, + 'not-tags': common_param, + 'not-tags-any': common_param, +}) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 2f2d1f9e67..d08aeda229 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -14,8 +14,6 @@ # License for the specific language governing permissions and limitations # under the License. -import re - from oslo_log import log as logging import oslo_messaging as messaging from oslo_utils import strutils @@ -188,6 +186,8 @@ class ServersController(wsgi.Controller): LOG.debug("Did not find any server create schemas") @extensions.expected_errors((400, 403)) + @validation.query_schema(schema_servers.query_params_v226, '2.26') + @validation.query_schema(schema_servers.query_params_v21, '2.1', '2.25') def index(self, req): """Returns a list of server names and ids for a given user.""" context = req.environ['nova.context'] @@ -199,6 +199,8 @@ class ServersController(wsgi.Controller): return servers @extensions.expected_errors((400, 403)) + @validation.query_schema(schema_servers.query_params_v226, '2.26') + @validation.query_schema(schema_servers.query_params_v21, '2.1', '2.25') def detail(self, req): """Returns a list of server details for a given user.""" context = req.environ['nova.context'] @@ -219,6 +221,13 @@ class ServersController(wsgi.Controller): remove_invalid_options(context, search_opts, self._get_server_search_options(req)) + for search_opt in search_opts: + if (search_opt in + schema_servers.JOINED_TABLE_QUERY_PARAMS_SERVERS.keys() or + search_opt.startswith('_')): + msg = _("Invalid filter field: %s.") % search_opt + raise exc.HTTPBadRequest(explanation=msg) + # Verify search by 'status' contains a valid status. # Convert it to filter by vm_state or task_state for compute_api. # For non-admin user, vm_state and task_state are filtered through @@ -242,12 +251,8 @@ class ServersController(wsgi.Controller): search_opts['task_state'] = task_state if 'changes-since' in search_opts: - try: - parsed = timeutils.parse_isotime(search_opts['changes-since']) - except ValueError: - msg = _('Invalid changes-since value') - raise exc.HTTPBadRequest(explanation=msg) - search_opts['changes-since'] = parsed + search_opts['changes-since'] = timeutils.parse_isotime( + search_opts['changes-since']) # By default, compute's get_all() will return deleted instances. # If an admin hasn't specified a 'deleted' search option, we need @@ -273,14 +278,6 @@ class ServersController(wsgi.Controller): msg = _("Only administrators may list deleted instances") raise exc.HTTPForbidden(explanation=msg) - # Verify the value of the 'name' option is a correct regex. - if 'name' in search_opts: - try: - re.compile(search_opts['name']) - except re.error: - msg = _("The regex for server name is incorrect") - raise exc.HTTPBadRequest(explanation=msg) - if api_version_request.is_supported(req, min_version='2.26'): for tag_filter in TAG_SEARCH_FILTERS: if tag_filter in search_opts: diff --git a/nova/api/validation/__init__.py b/nova/api/validation/__init__.py index 4581b42677..490c4ea870 100644 --- a/nova/api/validation/__init__.py +++ b/nova/api/validation/__init__.py @@ -17,6 +17,7 @@ Request Body validating middleware. """ import functools +import re from nova.api.openstack import api_version_request as api_version from nova.api.validation import validators @@ -119,13 +120,23 @@ def _strip_additional_query_parameters(schema, req): after _schema_validation_helper return `True`. """ additional_properties = schema.get('addtionalProperties', True) + pattern_regexes = [] + + patterns = schema.get('patternProperties', None) + if patterns: + for regex in patterns: + pattern_regexes.append(re.compile(regex)) if additional_properties: # `req.GET.keys` will return duplicated keys for multiple values # parameters. To get rid of duplicated keys, convert it to set. for param in set(req.GET.keys()): if param not in schema['properties'].keys(): - del req.GET[param] + # keys that can match the patternProperties will be + # retained and handle latter. + if not (list(regex for regex in pattern_regexes if + regex.match(param))): + del req.GET[param] def query_schema(query_params_schema, min_version=None, diff --git a/nova/api/validation/validators.py b/nova/api/validation/validators.py index 784dbc9485..39ad05f026 100644 --- a/nova/api/validation/validators.py +++ b/nova/api/validation/validators.py @@ -32,6 +32,17 @@ from nova import exception from nova.i18n import _ +@jsonschema.FormatChecker.cls_checks('regex') +def _validate_regex_format(instance): + if not isinstance(instance, six.text_type): + return False + try: + re.compile(instance) + except re.error: + return False + return True + + @jsonschema.FormatChecker.cls_checks('date-time') def _validate_datetime_format(instance): try: diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index 80d021f6a3..b9899eea52 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -579,7 +579,7 @@ class ServersControllerTest(ControllerTest): def test_get_servers_with_limit_bad_value(self): req = self.req('/fake/servers?limit=aaa') - self.assertRaises(webob.exc.HTTPBadRequest, + self.assertRaises(exception.ValidationError, self.controller.index, req) def test_get_server_details_empty(self): @@ -594,7 +594,7 @@ class ServersControllerTest(ControllerTest): def test_get_server_details_with_bad_name(self): req = self.req('/fake/servers/detail?name=%2Binstance') - self.assertRaises(webob.exc.HTTPBadRequest, + self.assertRaises(exception.ValidationError, self.controller.index, req) def test_get_server_details_with_limit(self): @@ -616,7 +616,7 @@ class ServersControllerTest(ControllerTest): def test_get_server_details_with_limit_bad_value(self): req = self.req('/fake/servers/detail?limit=aaa') - self.assertRaises(webob.exc.HTTPBadRequest, + self.assertRaises(exception.ValidationError, self.controller.detail, req) def test_get_server_details_with_limit_and_other_params(self): @@ -635,7 +635,7 @@ class ServersControllerTest(ControllerTest): href_parts = urlparse.urlparse(servers_links[0]['href']) self.assertEqual('/v2/fake/servers/detail', href_parts.path) params = urlparse.parse_qs(href_parts.query) - expected = {'limit': ['3'], 'blah': ['2:t'], + expected = {'limit': ['3'], 'sort_key': ['id1'], 'sort_dir': ['asc'], 'marker': [fakes.get_fake_uuid(2)]} self.assertThat(params, matchers.DictMatches(expected)) @@ -647,7 +647,7 @@ class ServersControllerTest(ControllerTest): def test_get_servers_with_bad_limit(self): req = self.req('/fake/servers?limit=asdf') - self.assertRaises(webob.exc.HTTPBadRequest, + self.assertRaises(exception.ValidationError, self.controller.index, req) def test_get_servers_with_marker(self): @@ -668,6 +668,16 @@ class ServersControllerTest(ControllerTest): self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) + def test_get_servers_with_invalid_filter_param(self): + req = self.req('/fake/servers?info_cache=asdf', + use_admin_context=True) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.index, req) + req = self.req('/fake/servers?__foo__=asdf', + use_admin_context=True) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.index, req) + def test_get_servers_with_bad_option(self): server_uuid = uuids.fake @@ -1067,7 +1077,8 @@ class ServersControllerTest(ControllerTest): def test_get_servers_allows_changes_since_bad_value(self): params = 'changes-since=asdf' req = self.req('/fake/servers?%s' % params) - self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, req) + self.assertRaises(exception.ValidationError, self.controller.index, + req) def test_get_servers_admin_filters_as_user(self): """Test getting servers by admin-only or unknown options when @@ -1116,7 +1127,7 @@ class ServersControllerTest(ControllerTest): self.assertIn('vm_state', search_opts) # Allowed only by admins with admin API on self.assertIn('ip', search_opts) - self.assertIn('unknown_option', search_opts) + self.assertNotIn('unknown_option', search_opts) return objects.InstanceList( objects=[fakes.stub_instance_obj(100, uuid=server_uuid)]) diff --git a/nova/tests/unit/test_api_validation.py b/nova/tests/unit/test_api_validation.py index 1762971fe1..25c016a14d 100644 --- a/nova/tests/unit/test_api_validation.py +++ b/nova/tests/unit/test_api_validation.py @@ -36,6 +36,8 @@ query_schema = { 'format': 'uuid'}), 'foos': parameter_types.multi_params({'type': 'string'}) }, + 'patternProperties': { + "^_": parameter_types.multi_params({'type': 'string'})}, 'additionalProperties': True } @@ -47,6 +49,22 @@ class FakeQueryParametersController(object): return list(set(req.GET.keys())) +class RegexFormatFakeController(object): + + schema = { + 'type': 'object', + 'properties': { + 'foo': { + 'format': 'regex', + }, + }, + } + + @validation.schema(request_body_schema=schema) + def post(self, req, body): + return 'Validation succeeded.' + + class FakeRequest(object): api_version_request = api_version.APIVersionRequest("2.1") environ = {} @@ -118,7 +136,7 @@ class APIValidationTestCase(test.NoDBTestCase): if not req: req = FakeRequest() try: - method(body=body, req=req,) + method(body=body, req=req) except exception.ValidationError as ex: self.assertEqual(400, ex.kwargs['code']) if not re.match(expected_detail, ex.kwargs['detail']): @@ -254,7 +272,7 @@ class QueryParamsSchemaTestCase(test.NoDBTestCase): def test_strip_out_additional_properties(self): req = fakes.HTTPRequest.blank( - "/tests?foos=abc&foo=%s&bar=123&bar=456" % fakes.FAKE_UUID) + "/tests?foos=abc&foo=%s&bar=123&-bar=456" % fakes.FAKE_UUID) req.api_version_request = api_version.APIVersionRequest("2.3") res = self.controller.get(req) res.sort() @@ -271,6 +289,14 @@ class QueryParamsSchemaTestCase(test.NoDBTestCase): res.sort() self.assertEqual(['bar', 'foo', 'foos'], res) + def test_strip_out_correct_pattern_retained(self): + req = fakes.HTTPRequest.blank( + "/tests?foos=abc&foo=%s&bar=123&_foo_=456" % fakes.FAKE_UUID) + req.api_version_request = api_version.APIVersionRequest("2.3") + res = self.controller.get(req) + res.sort() + self.assertEqual(['_foo_', 'foo', 'foos'], res) + class RequiredDisableTestCase(APIValidationTestCase): @@ -1414,3 +1440,24 @@ class Base64TestCase(APIValidationTestCase): "Value: %s. '%s' is not a 'base64'") % (value, value) self.check_validation_error(self.post, body={'foo': value}, expected_detail=detail) + + +class RegexFormatTestCase(APIValidationTestCase): + + def setUp(self): + super(APIValidationTestCase, self).setUp() + self.controller = RegexFormatFakeController() + + def test_validate_regex(self): + req = fakes.HTTPRequest.blank("") + self.assertEqual('Validation succeeded.', + self.controller.post(req, body={'foo': u'Myserver'})) + + def test_validate_regex_fails(self): + value = 1 + req = fakes.HTTPRequest.blank("") + detail = ("Invalid input for field/attribute foo. " + "Value: %s. %s is not a 'regex'") % (value, value) + self.check_validation_error(self.controller.post, req=req, + body={'foo': value}, + expected_detail=detail)