From b4ee36ef11ff9f4bf5e661005a0135d3f6396fb0 Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 9 Oct 2014 00:00:34 +0530 Subject: [PATCH] Fix bug in object name constraint SOF can support object names of upto 1024 if it conforms to the following constraints: Object names can have forward slashes ('/') in them. Each segment in between these slashes cannot exceed 255 characters with exception of the last segment which cannot exceed 221 characters. This constraint arises from the fact that each segment except the last one in object name is a directory, the last segment being an actual file on the filesystem. Also, restored default constraint values in swift.conf since the SOF constraints middleware (always in proxy pipeline) will take care of it. Change-Id: Ia7dc44671a87911c092fecd0344eace92f5c225b Signed-off-by: Prashanth Pai --- .functests | 9 ++++ etc/swift.conf-swiftonfile | 16 ++++-- swiftonfile/swift/common/constraints.py | 34 +++++++++---- .../common/middleware/check_constraints.py | 2 +- test/functional/swift_on_file_tests.py | 23 ++++++++- test/functional/test_account.py | 3 ++ test/functional/test_container.py | 3 ++ test/functional/tests.py | 6 +++ test/unit/common/test_constraints.py | 50 +++++++++++++++---- 9 files changed, 120 insertions(+), 26 deletions(-) diff --git a/.functests b/.functests index 0a7c6a8..009aee3 100755 --- a/.functests +++ b/.functests @@ -59,6 +59,15 @@ Before proceeding forward, please make sure you already have: name = swiftonfile default = yes + Added sof_constraints middleware in proxy pipeline + + [pipeline:main] + pipeline = catch_errors sof_constraints cache proxy-server + + [filter:sof_constraints] + use = egg:swiftonfile#sof_constraints + policies=swiftonfile + 4. Copied etc/object-server.conf-swiftonfile to /etc/swift/object-server/5.conf 5. Generated ring files for swiftonfile policy. diff --git a/etc/swift.conf-swiftonfile b/etc/swift.conf-swiftonfile index 4bbec24..7b801c1 100644 --- a/etc/swift.conf-swiftonfile +++ b/etc/swift.conf-swiftonfile @@ -107,7 +107,12 @@ name = swiftonfile # max_object_name_length is the max number of bytes in the utf8 encoding # of an object name -max_object_name_length = 221 +# max_object_name_length = 221 +# 221 is in fact the max possible length of the last segment of object name. +# Each segment is separated by a '/'. +# For example: If object name is "abc/def/ghi/jkl", then abc,def,ghi are all +# directories and "jkl" would be the file. +# # Why 221 ? # The longest filename supported by XFS in 255. # http://lxr.free-electrons.com/source/fs/xfs/xfs_types.h#L125 @@ -115,17 +120,18 @@ max_object_name_length = 221 # .OBJECT_NAME. # The random string is 32 character long and and file name has two dots. # Hence 255 - 32 - 2 = 221 -# NOTE: This limitation can be sefely raised by having slashes in really long -# object name. Each segment between slashes ('/') should not exceed 221. +# +# NOTE: Each segment between slashes ('/') should not exceed 255 and the last +# segment should not exceed 221. # max_account_name_length is the max number of bytes in the utf8 encoding # of an account name -max_account_name_length = 255 +# max_account_name_length = 255 # max_container_name_length is the max number of bytes in the utf8 encoding # of a container name -max_container_name_length = 255 +# max_container_name_length = 255 # Why 255 ? # The longest filename supported by XFS in 255. diff --git a/swiftonfile/swift/common/constraints.py b/swiftonfile/swift/common/constraints.py index 7d98feb..adcd2a3 100644 --- a/swiftonfile/swift/common/constraints.py +++ b/swiftonfile/swift/common/constraints.py @@ -16,8 +16,16 @@ import os from swift.common.swob import HTTPBadRequest -SOF_MAX_CONTAINER_NAME_LENGTH = 255 -SOF_MAX_OBJECT_NAME_LENGTH = 221 +SOF_MAX_DIR_NAME_LENGTH = 255 +# A container is also a directory on the fileystem with the same name. Hence: +SOF_MAX_CONTAINER_NAME_LENGTH = SOF_MAX_DIR_NAME_LENGTH + +SOF_MAX_OBJECT_FILENAME_LENGTH = 221 +# SOF_MAX_OBJECT_FILENAME_LENGTH is the length of the last segment of object +# name. Each 'segment/component' is separated by a '/'. +# For example: If object name is "abc/def/ghi/jkl", then abc,def,ghi are all +# directories and "jkl" would be the file. This file name cannot exceed +# SOF_MAX_OBJECT_FILENAME_LENGTH. # Why 221 ? # The longest filename supported by XFS in 255. # http://lxr.free-electrons.com/source/fs/xfs/xfs_types.h#L125 @@ -25,15 +33,19 @@ SOF_MAX_OBJECT_NAME_LENGTH = 221 # .OBJECT_NAME. # The random string is 32 character long and and file name has two dots. # Hence 255 - 32 - 2 = 221 -# NOTE: This limitation can be sefely raised by having slashes in really long -# object name. Each segment between slashes ('/') should not exceed 221. +# NOTE: Each segment between slashes ('/') should not exceed 255 and the last +# segment should not exceed 221. -def validate_obj_name_component(obj): +def validate_obj_name_component(obj, last_component=False): if not obj: return 'cannot begin, end, or have contiguous %s\'s' % os.path.sep - if len(obj) > SOF_MAX_OBJECT_NAME_LENGTH: - return 'too long (%d)' % len(obj) + if not last_component: + if len(obj) > SOF_MAX_DIR_NAME_LENGTH: + return 'too long (%d)' % len(obj) + else: + if len(obj) > SOF_MAX_OBJECT_FILENAME_LENGTH: + return 'too long (%d)' % len(obj) if obj == '.' or obj == '..': return 'cannot be . or ..' return '' @@ -55,8 +67,12 @@ def check_object_creation(req, object_name): """ # SoF's additional checks ret = None - for obj in object_name.split(os.path.sep): - reason = validate_obj_name_component(obj) + object_name_components = object_name.split(os.path.sep) + last_component = False + for i, obj in enumerate(object_name_components): + if i == (len(object_name_components) - 1): + last_component = True + reason = validate_obj_name_component(obj, last_component) if reason: bdy = 'Invalid object name "%s", component "%s" %s' \ % (object_name, obj, reason) diff --git a/swiftonfile/swift/common/middleware/check_constraints.py b/swiftonfile/swift/common/middleware/check_constraints.py index ee26ee5..3a6557d 100644 --- a/swiftonfile/swift/common/middleware/check_constraints.py +++ b/swiftonfile/swift/common/middleware/check_constraints.py @@ -28,7 +28,7 @@ For example:: pipeline = catch_errors sof_constraints cache proxy-server [filter:sof_constraints] - use = egg:swift#sof_constraints + use = egg:swiftonfile#sof_constraints policies=swiftonfile,gold """ diff --git a/test/functional/swift_on_file_tests.py b/test/functional/swift_on_file_tests.py index 3537c2f..90d058e 100644 --- a/test/functional/swift_on_file_tests.py +++ b/test/functional/swift_on_file_tests.py @@ -127,7 +127,8 @@ class TestSwiftOnFile(Base): file_item.write_random() self.assert_status(201) file_info = file_item.info() - fhOnMountPoint = open(os.path.join(self.env.root_dir, + fhOnMountPoint = open(os.path.join( + self.env.root_dir, 'AUTH_' + self.env.account.name, self.env.container.name, file_name), 'r') @@ -158,3 +159,23 @@ class TestSwiftOnFile(Base): # Confirm that Etag is present in response headers self.assert_(data_hash == object_item.info()['etag']) self.assert_status(200) + + def testObjectNameConstraints(self): + valid_object_names = ["a/b/c/d", + '/'.join(("1@3%&*0-", "};+=]|")), + '/'.join(('a' * 20, 'b' * 20, 'c' * 20))] + for object_name in valid_object_names: + file_item = self.env.container.file(object_name) + file_item.write_random() + self.assert_status(201) + + invalid_object_names = ["a/./b", + "a/b/../d", + "a//b", + "a/c//", + '/'.join(('a' * 256, 'b' * 255, 'c' * 221)), + '/'.join(('a' * 255, 'b' * 255, 'c' * 222))] + + for object_name in invalid_object_names: + file_item = self.env.container.file(object_name) + self.assertRaises(ResponseError, file_item.write) # 503 or 400 diff --git a/test/functional/test_account.py b/test/functional/test_account.py index b6b279d..0b0ccff 100755 --- a/test/functional/test_account.py +++ b/test/functional/test_account.py @@ -741,6 +741,9 @@ class TestAccount(unittest.TestCase): self.assertEqual(resp.getheader('x-account-meta-two'), '2') def test_bad_metadata(self): + + raise SkipTest('SOF constraints middleware enforces constraints.') + if tf.skip: raise SkipTest diff --git a/test/functional/test_container.py b/test/functional/test_container.py index 3a6e1b9..969b9bf 100755 --- a/test/functional/test_container.py +++ b/test/functional/test_container.py @@ -368,6 +368,9 @@ class TestContainer(unittest.TestCase): self.assertEqual(resp.status, 404) def test_POST_bad_metadata(self): + + raise SkipTest('SOF constraints middleware enforces constraints.') + if tf.skip: raise SkipTest diff --git a/test/functional/tests.py b/test/functional/tests.py index bbe370f..924f193 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -351,6 +351,9 @@ class TestContainer(Base): set_up = False def testContainerNameLimit(self): + + raise SkipTest('SOF constraints middleware enforces constraints.') + limit = load_constraint('max_container_name_length') for l in (limit - 100, limit - 10, limit - 1, limit, @@ -971,6 +974,9 @@ class TestFile(Base): self.assert_status(404) def testNameLimit(self): + + raise SkipTest('SOF constraints middleware enforces constraints.') + limit = load_constraint('max_object_name_length') for l in (1, 10, limit / 2, limit - 1, limit, limit + 1, limit * 2): diff --git a/test/unit/common/test_constraints.py b/test/unit/common/test_constraints.py index a9ec7d9..a7a8f96 100644 --- a/test/unit/common/test_constraints.py +++ b/test/unit/common/test_constraints.py @@ -14,7 +14,7 @@ # limitations under the License. import unittest -from mock import Mock, patch +from mock import Mock from swiftonfile.swift.common import constraints as cnt @@ -26,16 +26,32 @@ class TestConstraints(unittest.TestCase): """ Tests for common.constraints """ def test_validate_obj_name_component(self): - max_obj_len = cnt.SOF_MAX_OBJECT_NAME_LENGTH - self.assertFalse( - cnt.validate_obj_name_component('tests' * (max_obj_len / 5))) - self.assertEqual(cnt.validate_obj_name_component( - 'tests' * 60), 'too long (300)') + + # Non-last object name component - success + for i in (220, 221, 222, 254, 255): + obj_comp_name = 'a' * i + self.assertFalse(cnt.validate_obj_name_component(obj_comp_name)) + + # Last object name component - success + for i in (220, 221): + obj_comp_name = 'a' * i + self.assertFalse( + cnt.validate_obj_name_component(obj_comp_name, True)) def test_validate_obj_name_component_err(self): - max_obj_len = cnt.SOF_MAX_OBJECT_NAME_LENGTH - self.assertTrue(cnt.validate_obj_name_component( - 'tests' * (max_obj_len / 5 + 1))) + + # Non-last object name component - err + for i in (256, 257): + obj_comp_name = 'a' * i + result = cnt.validate_obj_name_component(obj_comp_name) + self.assertEqual(result, "too long (%d)" % i) + + # Last object name component - err + for i in (222, 223): + obj_comp_name = 'a' * i + result = cnt.validate_obj_name_component(obj_comp_name, True) + self.assertEqual(result, "too long (%d)" % i) + self.assertTrue(cnt.validate_obj_name_component('.')) self.assertTrue(cnt.validate_obj_name_component('..')) self.assertTrue(cnt.validate_obj_name_component('')) @@ -43,4 +59,18 @@ class TestConstraints(unittest.TestCase): def test_check_object_creation(self): req = Mock() req.headers = [] - self.assertFalse(cnt.check_object_creation(req, 'dir/z')) + + valid_object_names = ["a/b/c/d", + '/'.join(("1@3%&*0-", "};+=]|")), + '/'.join(('a' * 255, 'b' * 255, 'c' * 221))] + for o in valid_object_names: + self.assertFalse(cnt.check_object_creation(req, o)) + + invalid_object_names = ["a/./b", + "a/b/../d", + "a//b", + "a/c//", + '/'.join(('a' * 256, 'b' * 255, 'c' * 221)), + '/'.join(('a' * 255, 'b' * 255, 'c' * 222))] + for o in invalid_object_names: + self.assertTrue(cnt.check_object_creation(req, o))