From 4aba2fbb25edf8936e00ee9f5736cc2c0c383c32 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Fri, 27 Mar 2015 15:50:38 -0700 Subject: [PATCH] Check if REST API version is valid Swift doesn't check if the used API version is valid. Currently there is only one valid REST API version, but that might change in the future. This patch enforces "v1" or "v1.0" as the version string when accessing account, containers and objects. The list of accepted version strings can be manually overridden using a comma-separated list in swift.conf to make this backward-compatible. The constraint loader has been modified slightly to accept strings as well as integers. Any request to an account, container, and object which does not provide the correct version string will get a 400 BadRequest response. The allowed api versions are by default excluded from /info. Co-Authored-By: Christian Schwede Co-Authored-By: John Dickinson Closes Bug #1437442 Change-Id: I5ab6e236544378abf2eab562ab759513d09bc256 --- etc/proxy-server.conf-sample | 7 ++-- etc/swift.conf-sample | 11 ++++++ swift/common/constraints.py | 18 ++++++++- swift/common/exceptions.py | 4 ++ swift/proxy/server.py | 10 ++++- test/unit/proxy/test_server.py | 66 +++++++++++++++++++++++++++++++-- test/unit/proxy/test_sysmeta.py | 4 ++ 7 files changed, 110 insertions(+), 10 deletions(-) diff --git a/etc/proxy-server.conf-sample b/etc/proxy-server.conf-sample index 36b5b97d2e..37fc7d4564 100644 --- a/etc/proxy-server.conf-sample +++ b/etc/proxy-server.conf-sample @@ -17,9 +17,10 @@ bind_port = 8080 # to /info. You can withhold subsections by separating the dict level with a # ".". The following would cause the sections 'container_quotas' and 'tempurl' # to not be listed, and the key max_failed_deletes would be removed from -# bulk_delete. Default is empty, allowing all registered features to be listed -# via HTTP GET /info. -# disallowed_sections = container_quotas, tempurl, bulk_delete.max_failed_deletes +# bulk_delete. Default value is 'swift.valid_api_versions' which allows all +# registered features to be listed via HTTP GET /info except +# swift.valid_api_versions information +# disallowed_sections = swift.valid_api_versions, container_quotas, tempurl # Use an integer to override the number of pre-forked processes that will # accept connections. Should default to the number of effective cpu diff --git a/etc/swift.conf-sample b/etc/swift.conf-sample index 8726814012..f8accabaec 100644 --- a/etc/swift.conf-sample +++ b/etc/swift.conf-sample @@ -156,3 +156,14 @@ default = yes # of a container name #max_container_name_length = 256 + + +# By default all REST API calls should use "v1" or "v1.0" as the version string, +# for example "/v1/account". This can be manually overridden to make this +# backward-compatible, in case a different version string has been used before. +# Use a comma-separated list in case of multiple allowed versions, for example +# valid_api_versions = v0,v1,v2 +# This is only enforced for account, container and object requests. The allowed +# api versions are by default excluded from /info. + +# valid_api_versions = v1,v1.0 diff --git a/swift/common/constraints.py b/swift/common/constraints.py index 8e3ba53b00..4cee56ab3c 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -35,6 +35,7 @@ CONTAINER_LISTING_LIMIT = 10000 ACCOUNT_LISTING_LIMIT = 10000 MAX_ACCOUNT_NAME_LENGTH = 256 MAX_CONTAINER_NAME_LENGTH = 256 +VALID_API_VERSIONS = ["v1", "v1.0"] # If adding an entry to DEFAULT_CONSTRAINTS, note that # these constraints are automatically published by the @@ -52,6 +53,7 @@ DEFAULT_CONSTRAINTS = { 'account_listing_limit': ACCOUNT_LISTING_LIMIT, 'max_account_name_length': MAX_ACCOUNT_NAME_LENGTH, 'max_container_name_length': MAX_CONTAINER_NAME_LENGTH, + 'valid_api_versions': VALID_API_VERSIONS, } SWIFT_CONSTRAINTS_LOADED = False @@ -72,13 +74,17 @@ def reload_constraints(): SWIFT_CONSTRAINTS_LOADED = True for name in DEFAULT_CONSTRAINTS: try: - value = int(constraints_conf.get('swift-constraints', name)) + value = constraints_conf.get('swift-constraints', name) except NoOptionError: pass except NoSectionError: # We are never going to find the section for another option break else: + try: + value = int(value) + except ValueError: + value = utils.list_from_csv(value) OVERRIDE_CONSTRAINTS[name] = value for name, default in DEFAULT_CONSTRAINTS.items(): value = OVERRIDE_CONSTRAINTS.get(name, default) @@ -412,3 +418,13 @@ def check_account_format(req, account): request=req, body='Account name cannot contain slashes') return account + + +def valid_api_version(version): + """ Checks if the requested version is valid. + + Currently Swift only supports "v1" and "v1.0". """ + global VALID_API_VERSIONS + if not isinstance(VALID_API_VERSIONS, list): + VALID_API_VERSIONS = [str(VALID_API_VERSIONS)] + return version in VALID_API_VERSIONS diff --git a/swift/common/exceptions.py b/swift/common/exceptions.py index b4c926eb1c..dab0777d6d 100644 --- a/swift/common/exceptions.py +++ b/swift/common/exceptions.py @@ -199,6 +199,10 @@ class MimeInvalid(SwiftException): pass +class APIVersionError(SwiftException): + pass + + class ClientException(Exception): def __init__(self, msg, http_scheme='', http_host='', http_port='', diff --git a/swift/proxy/server.py b/swift/proxy/server.py index 8c9e223720..b631542f60 100644 --- a/swift/proxy/server.py +++ b/swift/proxy/server.py @@ -33,13 +33,14 @@ from swift.common.utils import cache_from_env, get_logger, \ get_remote_client, split_path, config_true_value, generate_trans_id, \ affinity_key_function, affinity_locality_predicate, list_from_csv, \ register_swift_info -from swift.common.constraints import check_utf8 +from swift.common.constraints import check_utf8, valid_api_version from swift.proxy.controllers import AccountController, ContainerController, \ ObjectControllerRouter, InfoController from swift.proxy.controllers.base import get_container_info from swift.common.swob import HTTPBadRequest, HTTPForbidden, \ HTTPMethodNotAllowed, HTTPNotFound, HTTPPreconditionFailed, \ HTTPServerError, HTTPException, Request, HTTPServiceUnavailable +from swift.common.exceptions import APIVersionError # List of entry points for mandatory middlewares. @@ -210,7 +211,7 @@ class Application(object): self.expose_info = config_true_value( conf.get('expose_info', 'yes')) self.disallowed_sections = list_from_csv( - conf.get('disallowed_sections')) + conf.get('disallowed_sections', 'swift.valid_api_versions')) self.admin_key = conf.get('admin_key', None) register_swift_info( version=swift_version, @@ -260,6 +261,8 @@ class Application(object): account_name=account, container_name=container, object_name=obj) + if account and not valid_api_version(version): + raise APIVersionError('Invalid path') if obj and container and account: info = get_container_info(req.environ, self) policy_index = req.headers.get('X-Backend-Storage-Policy-Index', @@ -340,6 +343,9 @@ class Application(object): p = req.path_info if isinstance(p, unicode): p = p.encode('utf-8') + except APIVersionError: + self.logger.increment('errors') + return HTTPBadRequest(request=req) except ValueError: self.logger.increment('errors') return HTTPNotFound(request=req) diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 901ff925b0..3319696eb7 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -30,7 +30,7 @@ from textwrap import dedent from urllib import quote from hashlib import md5 from pyeclib.ec_iface import ECDriverError -from tempfile import mkdtemp +from tempfile import mkdtemp, NamedTemporaryFile import weakref import operator import functools @@ -53,7 +53,8 @@ from swift.container import server as container_server from swift.obj import server as object_server from swift.common.middleware import proxy_logging from swift.common.middleware.acl import parse_acl, format_acl -from swift.common.exceptions import ChunkReadTimeout, DiskFileNotExist +from swift.common.exceptions import ChunkReadTimeout, DiskFileNotExist, \ + APIVersionError from swift.common import utils, constraints from swift.common.ring import RingData from swift.common.utils import mkdirs, normalize_timestamp, NullLogger @@ -881,7 +882,9 @@ class TestProxyServer(unittest.TestCase): self.assertTrue(app.expose_info) self.assertTrue(isinstance(app.disallowed_sections, list)) - self.assertEqual(0, len(app.disallowed_sections)) + self.assertEqual(1, len(app.disallowed_sections)) + self.assertEqual(['swift.valid_api_versions'], + app.disallowed_sections) self.assertTrue(app.admin_key is None) def test_get_info_controller(self): @@ -959,6 +962,58 @@ class TestProxyServer(unittest.TestCase): self.assertEqual(log_kwargs['exc_info'][1], e3) self.assertEqual(4, node_error_count(app, node)) + def test_valid_api_version(self): + app = proxy_server.Application({}, FakeMemcache(), + account_ring=FakeRing(), + container_ring=FakeRing()) + + # The version string is only checked for account, container and object + # requests; the raised APIVersionError returns a 404 to the client + for path in [ + '/v2/a', + '/v2/a/c', + '/v2/a/c/o']: + req = Request.blank(path) + self.assertRaises(APIVersionError, app.get_controller, req) + + # Default valid API versions are ok + for path in [ + '/v1/a', + '/v1/a/c', + '/v1/a/c/o', + '/v1.0/a', + '/v1.0/a/c', + '/v1.0/a/c/o']: + req = Request.blank(path) + controller, path_parts = app.get_controller(req) + self.assertTrue(controller is not None) + + # Ensure settings valid API version constraint works + for version in ["42", 42]: + try: + with NamedTemporaryFile() as f: + f.write('[swift-constraints]\n') + f.write('valid_api_versions = %s\n' % version) + f.flush() + with mock.patch.object(utils, 'SWIFT_CONF_FILE', f.name): + constraints.reload_constraints() + + req = Request.blank('/%s/a' % version) + controller, _ = app.get_controller(req) + self.assertTrue(controller is not None) + + # In this case v1 is invalid + req = Request.blank('/v1/a') + self.assertRaises(APIVersionError, app.get_controller, req) + finally: + constraints.reload_constraints() + + # Check that the valid_api_versions is not exposed by default + req = Request.blank('/info') + controller, path_parts = app.get_controller(req) + self.assertTrue('swift.valid_api_versions' in + path_parts.get('disallowed_sections')) + @patch_policies([ StoragePolicy(0, 'zero', is_default=True), @@ -8580,9 +8635,12 @@ class TestSwiftInfo(unittest.TestCase): self.assertTrue('strict_cors_mode' in si) self.assertEqual(si['allow_account_management'], False) self.assertEqual(si['account_autocreate'], False) + # This setting is by default excluded by disallowed_sections + self.assertEqual(si['valid_api_versions'], + constraints.VALID_API_VERSIONS) # this next test is deliberately brittle in order to alert if # other items are added to swift info - self.assertEqual(len(si), 16) + self.assertEqual(len(si), 17) self.assertTrue('policies' in si) sorted_pols = sorted(si['policies'], key=operator.itemgetter('name')) diff --git a/test/unit/proxy/test_sysmeta.py b/test/unit/proxy/test_sysmeta.py index a45c689abd..6b5727a461 100644 --- a/test/unit/proxy/test_sysmeta.py +++ b/test/unit/proxy/test_sysmeta.py @@ -144,11 +144,15 @@ class TestObjectSysmeta(unittest.TestCase): fake_http_connect(200), FakeServerConnection(self.obj_ctlr)) + self.orig_base_http_connect = swift.proxy.controllers.base.http_connect + self.orig_obj_http_connect = swift.proxy.controllers.obj.http_connect swift.proxy.controllers.base.http_connect = http_connect swift.proxy.controllers.obj.http_connect = http_connect def tearDown(self): shutil.rmtree(self.tmpdir) + swift.proxy.controllers.base.http_connect = self.orig_base_http_connect + swift.proxy.controllers.obj.http_connect = self.orig_obj_http_connect original_sysmeta_headers_1 = {'x-object-sysmeta-test0': 'val0', 'x-object-sysmeta-test1': 'val1'}