From 27a734c78aabdbf04977a01039004e471feae30c Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 27 Feb 2020 16:25:44 -0800 Subject: [PATCH] s3api: Allow CORS preflight requests Unfortunately, we can't identify the user, so we can't map to an account, so we can't respect whatever CORS metadata might be set on the container. As a result, the allowed origins must be configured cluster-wide. Add a new config option, cors_preflight_allow_origin, for that; default it to blank (ie, deny preflights from all origins, preserving existing behavior), but allow either a comma-separated list of origins or * (to allow all origins). Change-Id: I985143bf03125a05792e79bc5e5f83722d6431b3 Co-Authored-By: Matthew Oliver --- doc/saio/swift/proxy-server.conf | 1 + docker/rootfs/etc/swift/proxy-server.conf | 1 + etc/proxy-server.conf-sample | 6 ++ swift/common/middleware/s3api/s3api.py | 33 +++++++++- test/cors/test-s3-obj.js | 61 ++++++++++++++++--- test/unit/common/middleware/s3api/test_obj.py | 41 +++++++++++++ .../common/middleware/s3api/test_s3api.py | 29 +++++++++ 7 files changed, 161 insertions(+), 11 deletions(-) diff --git a/doc/saio/swift/proxy-server.conf b/doc/saio/swift/proxy-server.conf index 57a0540876..124b36c62b 100644 --- a/doc/saio/swift/proxy-server.conf +++ b/doc/saio/swift/proxy-server.conf @@ -92,6 +92,7 @@ use = egg:swift#symlink use = egg:swift#s3api s3_acl = yes check_bucket_owner = yes +cors_preflight_allow_origin = * # Example to create root secret: `openssl rand -base64 32` [filter:keymaster] diff --git a/docker/rootfs/etc/swift/proxy-server.conf b/docker/rootfs/etc/swift/proxy-server.conf index a964f15203..8189cb7f2d 100644 --- a/docker/rootfs/etc/swift/proxy-server.conf +++ b/docker/rootfs/etc/swift/proxy-server.conf @@ -82,6 +82,7 @@ use = egg:swift#symlink # To enable, add the s3api middleware to the pipeline before tempauth [filter:s3api] use = egg:swift#s3api +cors_preflight_allow_origin = * # Example to create root secret: `openssl rand -base64 32` [filter:keymaster] diff --git a/etc/proxy-server.conf-sample b/etc/proxy-server.conf-sample index b474b27a36..434ed9c627 100644 --- a/etc/proxy-server.conf-sample +++ b/etc/proxy-server.conf-sample @@ -629,6 +629,12 @@ use = egg:swift#s3api # AWS allows clock skew up to 15 mins; note that older versions of swift/swift3 # allowed at most 5 mins. # allowable_clock_skew = 900 +# +# CORS preflight requests don't contain enough information for us to +# identify the account that should be used for the real request, so +# the allowed origins must be set cluster-wide. (default: blank; all +# preflight requests will be denied) +# cors_preflight_allow_origin = # You can override the default log routing for this filter here: # log_name = s3api diff --git a/swift/common/middleware/s3api/s3api.py b/swift/common/middleware/s3api/s3api.py index c5d805358a..7b9ad07784 100644 --- a/swift/common/middleware/s3api/s3api.py +++ b/swift/common/middleware/s3api/s3api.py @@ -157,7 +157,7 @@ from swift.common.middleware.s3api.s3response import ErrorResponse, \ InternalError, MethodNotAllowed, S3ResponseBase, S3NotImplemented from swift.common.utils import get_logger, register_swift_info, \ config_true_value, config_positive_int_value, split_path, \ - closing_if_possible + closing_if_possible, list_from_csv from swift.common.middleware.s3api.utils import Config from swift.common.middleware.s3api.acl_handlers import get_acl_handler @@ -277,12 +277,43 @@ class S3ApiMiddleware(object): wsgi_conf.get('min_segment_size', 5242880)) self.conf.allowable_clock_skew = config_positive_int_value( wsgi_conf.get('allowable_clock_skew', 15 * 60)) + self.conf.cors_preflight_allow_origin = list_from_csv(wsgi_conf.get( + 'cors_preflight_allow_origin', '')) + if '*' in self.conf.cors_preflight_allow_origin and \ + len(self.conf.cors_preflight_allow_origin) > 1: + raise ValueError('if cors_preflight_allow_origin should include ' + 'all domains, * must be the only entry') self.logger = get_logger( wsgi_conf, log_route=wsgi_conf.get('log_name', 's3api')) self.check_pipeline(wsgi_conf) def __call__(self, env, start_response): + origin = env.get('HTTP_ORIGIN') + acrh = env.get('HTTP_ACCESS_CONTROL_REQUEST_HEADERS', '').lower() + if self.conf.cors_preflight_allow_origin and origin and \ + env['REQUEST_METHOD'] == 'OPTIONS' and \ + 'authorization' in acrh and \ + not env['PATH_INFO'].startswith(('/v1/', '/v1.0/')): + # I guess it's likely going to be an S3 request? *shrug* + if self.conf.cors_preflight_allow_origin != ['*'] and \ + origin not in self.conf.cors_preflight_allow_origin: + start_response('401 Unauthorized', [ + ('Allow', 'GET, HEAD, PUT, POST, DELETE, OPTIONS'), + ]) + return [b''] + + start_response('200 OK', [ + ('Allow', 'GET, HEAD, PUT, POST, DELETE, OPTIONS'), + ('Access-Control-Allow-Origin', origin), + ('Access-Control-Allow-Methods', + 'GET, HEAD, PUT, POST, DELETE, OPTIONS'), + ('Access-Control-Allow-Headers', + ', '.join(set(list_from_csv(acrh)))), + ('Vary', 'Origin, Access-Control-Request-Headers'), + ]) + return [b''] + try: req_class = get_request_class(env, self.conf.s3_acl) req = req_class(env, self.app, self.conf) diff --git a/test/cors/test-s3-obj.js b/test/cors/test-s3-obj.js index 35dd198158..dfa119ef9e 100644 --- a/test/cors/test-s3-obj.js +++ b/test/cors/test-s3-obj.js @@ -133,28 +133,69 @@ function makeTests (params) { Bucket: 'private-with-cors', Key: 'obj' }) - .then(CorsBlocked)], // Pre-flight failed - ['PUT', - () => MakeS3Request(service, 'putObject', { - Bucket: 'private-with-cors', - Key: 'put-target', - Body: 'test' - }) - .then(CorsBlocked)], // Pre-flight failed + .then(HasStatus(200, 'OK')) + .then(CheckS3Headers) + .then(HasHeaders(['x-amz-meta-mtime'])) + .then(DoesNotHaveHeaders(['X-Object-Meta-Mtime'])) + .then(HasHeaders({ + 'Content-Type': 'application/octet-stream', + Etag: '"0f343b0931126a20f133d67c2b018a3b"' + })) + .then(BodyHasLength(1024))], + ['PUT then DELETE', + () => Promise.resolve('put-target-' + Math.random()).then((objectName) => { + return MakeS3Request(service, 'putObject', { + Bucket: 'private-with-cors', + Key: objectName, + Body: 'test' + }) + .then(HasStatus(200, 'OK')) + .then(CheckS3Headers) + .then(HasHeaders({ + 'Content-Type': 'text/html; charset=UTF-8', + Etag: '"098f6bcd4621d373cade4e832627b4f6"' + })) + .then(HasNoBody) + .then((resp) => { + return MakeS3Request(service, 'deleteObject', { + Bucket: 'private-with-cors', + Key: objectName + }) + }) + .then(HasStatus(204, 'No Content')) + .then(CheckTransactionIdHeaders) + .then(HasNoBody) + })], ['GET If-Match matching', () => MakeS3Request(service, 'getObject', { Bucket: 'private-with-cors', Key: 'obj', IfMatch: '0f343b0931126a20f133d67c2b018a3b' }) - .then(CorsBlocked)], // Pre-flight failed + .then(HasStatus(200, 'OK')) + .then(CheckS3Headers) + .then(HasHeaders(['x-amz-meta-mtime'])) + .then(DoesNotHaveHeaders(['X-Object-Meta-Mtime'])) + .then(HasHeaders({ + 'Content-Type': 'application/octet-stream', + Etag: '"0f343b0931126a20f133d67c2b018a3b"' + })) + .then(BodyHasLength(1024))], ['GET Range', () => MakeS3Request(service, 'getObject', { Bucket: 'private-with-cors', Key: 'obj', Range: 'bytes=100-199' }) - .then(CorsBlocked)], // Pre-flight failed + .then(HasStatus(206, 'Partial Content')) + .then(CheckS3Headers) + .then(HasHeaders(['x-amz-meta-mtime'])) + .then(DoesNotHaveHeaders(['X-Object-Meta-Mtime'])) + .then(HasHeaders({ + 'Content-Type': 'application/octet-stream', + Etag: '"0f343b0931126a20f133d67c2b018a3b"' + })) + .then(BodyHasLength(100))] ] } diff --git a/test/unit/common/middleware/s3api/test_obj.py b/test/unit/common/middleware/s3api/test_obj.py index fcf3b78c92..6b6d0ce767 100644 --- a/test/unit/common/middleware/s3api/test_obj.py +++ b/test/unit/common/middleware/s3api/test_obj.py @@ -1724,6 +1724,47 @@ class TestS3ApiObj(S3ApiTestCase): 'test:write', 'READ', src_path='') self.assertEqual(status.split()[0], '400') + def test_cors_preflight(self): + req = Request.blank( + '/bucket/cors-object', + environ={'REQUEST_METHOD': 'OPTIONS'}, + headers={'Origin': 'http://example.com', + 'Access-Control-Request-Method': 'GET', + 'Access-Control-Request-Headers': 'authorization'}) + self.s3api.conf.cors_preflight_allow_origin = ['*'] + status, headers, body = self.call_s3api(req) + self.assertEqual(status, '200 OK') + self.assertDictEqual(headers, { + 'Allow': 'GET, HEAD, PUT, POST, DELETE, OPTIONS', + 'Access-Control-Allow-Origin': 'http://example.com', + 'Access-Control-Allow-Methods': ('GET, HEAD, PUT, POST, DELETE, ' + 'OPTIONS'), + 'Access-Control-Allow-Headers': 'authorization', + 'Vary': 'Origin, Access-Control-Request-Headers', + }) + + # test more allow_origins + self.s3api.conf.cors_preflight_allow_origin = ['http://example.com', + 'http://other.com'] + status, headers, body = self.call_s3api(req) + self.assertEqual(status, '200 OK') + self.assertDictEqual(headers, { + 'Allow': 'GET, HEAD, PUT, POST, DELETE, OPTIONS', + 'Access-Control-Allow-Origin': 'http://example.com', + 'Access-Control-Allow-Methods': ('GET, HEAD, PUT, POST, DELETE, ' + 'OPTIONS'), + 'Access-Control-Allow-Headers': 'authorization', + 'Vary': 'Origin, Access-Control-Request-Headers', + }) + + # Wrong protocol + self.s3api.conf.cors_preflight_allow_origin = ['https://example.com'] + status, headers, body = self.call_s3api(req) + self.assertEqual(status, '401 Unauthorized') + self.assertEqual(headers, { + 'Allow': 'GET, HEAD, PUT, POST, DELETE, OPTIONS', + }) + def test_cors_headers(self): # note: Access-Control-Allow-Methods would normally be expected in # response to an OPTIONS request but its included here in GET/PUT tests diff --git a/test/unit/common/middleware/s3api/test_s3api.py b/test/unit/common/middleware/s3api/test_s3api.py index e530e70714..fb9b0f4d85 100644 --- a/test/unit/common/middleware/s3api/test_s3api.py +++ b/test/unit/common/middleware/s3api/test_s3api.py @@ -118,6 +118,7 @@ class TestS3ApiMiddleware(S3ApiTestCase): 'min_segment_size': 5242880, 'multi_delete_concurrency': 2, 's3_acl': False, + 'cors_preflight_allow_origin': [], }) s3api = S3ApiMiddleware(None, {}) self.assertEqual(expected, s3api.conf) @@ -140,10 +141,38 @@ class TestS3ApiMiddleware(S3ApiTestCase): 'min_segment_size': 1000000, 'multi_delete_concurrency': 1, 's3_acl': True, + 'cors_preflight_allow_origin': 'foo.example.com,bar.example.com', } s3api = S3ApiMiddleware(None, conf) + conf['cors_preflight_allow_origin'] = \ + conf['cors_preflight_allow_origin'].split(',') self.assertEqual(conf, s3api.conf) + # test allow_origin list with a '*' fails. + conf = { + 'storage_domain': 'somewhere', + 'location': 'us-west-1', + 'force_swift_request_proxy_log': True, + 'dns_compliant_bucket_names': False, + 'allow_multipart_uploads': False, + 'allow_no_owner': True, + 'allowable_clock_skew': 300, + 'auth_pipeline_check': False, + 'check_bucket_owner': True, + 'max_bucket_listing': 500, + 'max_multi_delete_objects': 600, + 'max_parts_listing': 70, + 'max_upload_part_num': 800, + 'min_segment_size': 1000000, + 'multi_delete_concurrency': 1, + 's3_acl': True, + 'cors_preflight_allow_origin': 'foo.example.com,bar.example.com,*', + } + with self.assertRaises(ValueError) as ex: + S3ApiMiddleware(None, conf) + self.assertIn("if cors_preflight_allow_origin should include all " + "domains, * must be the only entry", str(ex.exception)) + def check_bad_positive_ints(**kwargs): bad_conf = dict(conf, **kwargs) self.assertRaises(ValueError, S3ApiMiddleware, None, bad_conf)