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 <christian.schwede@enovance.com>
Co-Authored-By: John Dickinson <me@not.mn>

Closes Bug #1437442

Change-Id: I5ab6e236544378abf2eab562ab759513d09bc256

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

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

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

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='',

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)

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'))

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'}