From 2abb829a5dd965b99bc8f18f13c3bfbe1ece27b2 Mon Sep 17 00:00:00 2001
From: David Goetz <dpgoetz@gmail.com>
Date: Wed, 15 Jan 2014 14:49:31 -0800
Subject: [PATCH] Make cors work better.
CORS doesn't really work with swift right now. OPTIONS calls for the most part
work but for so called "simple cross-site requests" (i.e. those that don't
require a pre-flight OPTIONS request) Swift always returns the Origin it was
given as the Access-Control-Allow-Origin in the response. This makes CORS
"work" for these requests but if you actually wanted the javascript user agent
to restrict anything for you it wouldn't be able to!
You can duplicate the issue with updated CORS test page:
http://docs.openstack.org/developer/swift/cors.html#test-cors-page
And a public container with an 'X-Container-Meta-Access-Control-Allow-Origin'
that does NOT match the webserver hosting the test-cors-page.
e.g.
with a public container that accepts cross-site requests from "example.com":
`swift post cors-container -m access-control-allow-origin:example.com -r .r:*`
You could point your browser at a copy of the test-cors-page on your
filesystem (the browser will will send 'Origin: null')
Without a token the XMLHttpRequest will not request any custom headers (i.e.
Access-Control-Request-Headers: x-auth-token) and the request will be made
with-out a preflight OPTIONS request (which Swift would have denied anyway
because the origin's don't match)
i.e. fill in "http://saio:8080/v1/AUTH_test/cors-container" for "URL" and
leave "Token" blank.
You would expect that the browser would not complete the request because
"Origin: null" does not match the configured "Access-Control-Allow-Origin:
example.com" on the container metadata, and indeed with this patch - it won't!
Also:
The way cors is set up does not play well with certain applications for swift.
If you are running a CDN on top of swift and you have the
Access-Control-Allow-Origin cors header set to * then you probably want the *
to be cached on the the CDN, not the Origin that happened to result in an
origin request.
Also:
If you were unfortunate enough to allow cors headers to be saved directly
onto objects then this allows them to supersede the headers coming from the
container.
NOTE: There is a change is behavior with this patch. Because its cors, a
spec that was created only to cause annoyance to all, I'll write out
what's being changed and hopefully someone will speak up if it breaks
there stuff.
previous behavior: When a request was made with a Origin header set the
cors_validation decorator would always add that origin as
the Access-Control-Allow-Origin header in the response-
whether the passed origin was a match with the container's
X-Container-Meta-Access-Control-Allow-Origin or not, or even
if the container did not have CORS set up at all.
new behavior: If strict_cors_mode is set to True in the proxy-server.conf
(which is the default) the cors_validation decorator will only
add the Access-Control-Allow-Origin header to the response when
the request's Origin matches the value set in
X-Container-Meta-Access-Control-Allow-Origin. NOTE- if the
container does not have CORS set up it won't just magically start
working. Furthremore, if the Origin doesn't match the
Access-Control-Allow-Origin - a successfully authorized request
(either by token or public ACL) won't be *denied* - it just
won't include the Access-Control-Allow-Origin header (it's up
to the security model in the browser to cancel the request
if the response doesn't include a matching Allow-Origin
header). On the other hand, if you want to restrict requests
with CORS, you can actually do it now.
If you are worried about breaking current functionality you
must set:
strict_cors_mode = False
in the proxy-server.conf. This will continue with returning the
passed in Origin as the Access-Control-Allow-Origin in the
response.
previous: If you had X-Container-Meta-Access-Control-Allow-Origin set to *
and you passed in Origin: http://hey.com you'd get
Access-Control-Allow-Origin: http://hey.com back. This was true for
both OPTIONS and regular reqs.
new: With X-Container-Meta-Access-Control-Allow-Origin set to * you get * back
for both OPTIONS and regular reqs.
previous: cors headers saved directly onto objects (by allowing them to be
saved via the allowed_headers config in the object-server conf)
would be overridden by whatever container cors you have set up.
new: For regular (non-OPTIONS) calls the object headers will be kept. The
container cors will only be applied to objects without the
'Access-Control-Allow-Origin' and 'Access-Control-Expose-Headers' headers.
This behavior doesn't make a whole lot of sense for OPTIONS calls so I
left that as is. I don't think that allowing cors headers to be saved
directly onto objects is a good idea and it should be discouraged.
DocImpact
Change-Id: I9b0219407e77c77a9bb1133cbcb179a4c681c4a8
---
doc/source/cors.rst | 5 +-
swift/common/wsgi.py | 3 +-
swift/proxy/controllers/base.py | 47 +++++++++-----
swift/proxy/server.py | 5 +-
test/functional/test_object.py | 112 ++++++++++++++++++++++++++++++++
test/unit/proxy/test_server.py | 55 ++++++++++++++--
6 files changed, 201 insertions(+), 26 deletions(-)
diff --git a/doc/source/cors.rst b/doc/source/cors.rst
index 15bdf10ce5..c17ccd7f82 100644
--- a/doc/source/cors.rst
+++ b/doc/source/cors.rst
@@ -134,7 +134,10 @@ Test CORS Page
}
request.open(method, url);
- request.setRequestHeader('X-Auth-Token', token);
+ if (token != '') {
+ // custom headers always trigger a pre-flight request
+ request.setRequestHeader('X-Auth-Token', token);
+ }
request.send(null);
}
</script>
diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py
index 7feb25d0b2..4f30a6f59e 100644
--- a/swift/common/wsgi.py
+++ b/swift/common/wsgi.py
@@ -572,7 +572,8 @@ def make_env(env, method=None, path=None, agent='Swift', query_string=None,
newenv = {}
for name in ('eventlet.posthooks', 'HTTP_USER_AGENT', 'HTTP_HOST',
'PATH_INFO', 'QUERY_STRING', 'REMOTE_USER', 'REQUEST_METHOD',
- 'SCRIPT_NAME', 'SERVER_NAME', 'SERVER_PORT', 'HTTP_ORIGIN',
+ 'SCRIPT_NAME', 'SERVER_NAME', 'SERVER_PORT',
+ 'HTTP_ORIGIN', 'HTTP_ACCESS_CONTROL_REQUEST_METHOD',
'SERVER_PROTOCOL', 'swift.cache', 'swift.source',
'swift.trans_id'):
if name in env:
diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py
index 7aaf2d5f8d..139d6609f0 100644
--- a/swift/proxy/controllers/base.py
+++ b/swift/proxy/controllers/base.py
@@ -212,6 +212,10 @@ def cors_validation(func):
# Call through to the decorated method
resp = func(*a, **kw)
+ if controller.app.strict_cors_mode and \
+ not controller.is_origin_allowed(cors_info, req_origin):
+ return resp
+
# Expose,
# - simple response headers,
# http://www.w3.org/TR/cors/#simple-response-header
@@ -219,24 +223,32 @@ def cors_validation(func):
# - user metadata headers
# - headers provided by the user in
# x-container-meta-access-control-expose-headers
- expose_headers = ['cache-control', 'content-language',
- 'content-type', 'expires', 'last-modified',
- 'pragma', 'etag', 'x-timestamp', 'x-trans-id']
- for header in resp.headers:
- if header.startswith('X-Container-Meta') or \
- header.startswith('X-Object-Meta'):
- expose_headers.append(header.lower())
- if cors_info.get('expose_headers'):
- expose_headers.extend(
- [header_line.strip()
- for header_line in cors_info['expose_headers'].split(' ')
- if header_line.strip()])
- resp.headers['Access-Control-Expose-Headers'] = \
- ', '.join(expose_headers)
+ if 'Access-Control-Expose-Headers' not in resp.headers:
+ expose_headers = [
+ 'cache-control', 'content-language', 'content-type',
+ 'expires', 'last-modified', 'pragma', 'etag',
+ 'x-timestamp', 'x-trans-id']
+ for header in resp.headers:
+ if header.startswith('X-Container-Meta') or \
+ header.startswith('X-Object-Meta'):
+ expose_headers.append(header.lower())
+ if cors_info.get('expose_headers'):
+ expose_headers.extend(
+ [header_line.strip()
+ for header_line in
+ cors_info['expose_headers'].split(' ')
+ if header_line.strip()])
+ resp.headers['Access-Control-Expose-Headers'] = \
+ ', '.join(expose_headers)
# The user agent won't process the response if the Allow-Origin
# header isn't included
- resp.headers['Access-Control-Allow-Origin'] = req_origin
+ if 'Access-Control-Allow-Origin' not in resp.headers:
+ if cors_info['allow_origin'] and \
+ cors_info['allow_origin'].strip() == '*':
+ resp.headers['Access-Control-Allow-Origin'] = '*'
+ else:
+ resp.headers['Access-Control-Allow-Origin'] = req_origin
return resp
else:
@@ -1256,7 +1268,10 @@ class Controller(object):
list_from_csv(req.headers['Access-Control-Request-Headers']))
# Populate the response with the CORS preflight headers
- headers['access-control-allow-origin'] = req_origin_value
+ if cors.get('allow_origin', '').strip() == '*':
+ headers['access-control-allow-origin'] = '*'
+ else:
+ headers['access-control-allow-origin'] = req_origin_value
if cors.get('max_age') is not None:
headers['access-control-max-age'] = cors.get('max_age')
headers['access-control-allow-methods'] = \
diff --git a/swift/proxy/server.py b/swift/proxy/server.py
index d42688b423..5b4a5b7b2d 100644
--- a/swift/proxy/server.py
+++ b/swift/proxy/server.py
@@ -130,6 +130,8 @@ class Application(object):
a.strip()
for a in conf.get('cors_allow_origin', '').split(',')
if a.strip()]
+ self.strict_cors_mode = config_true_value(
+ conf.get('strict_cors_mode', 't'))
self.node_timings = {}
self.timing_expiry = int(conf.get('timing_expiry', 300))
self.sorting_method = conf.get('sorting_method', 'shuffle').lower()
@@ -210,7 +212,8 @@ class Application(object):
container_listing_limit=constraints.CONTAINER_LISTING_LIMIT,
max_account_name_length=constraints.MAX_ACCOUNT_NAME_LENGTH,
max_container_name_length=constraints.MAX_CONTAINER_NAME_LENGTH,
- max_object_name_length=constraints.MAX_OBJECT_NAME_LENGTH)
+ max_object_name_length=constraints.MAX_OBJECT_NAME_LENGTH,
+ strict_cors_mode=self.strict_cors_mode)
def check_config(self):
"""
diff --git a/test/functional/test_object.py b/test/functional/test_object.py
index dad8635da9..9b999b6b90 100755
--- a/test/functional/test_object.py
+++ b/test/functional/test_object.py
@@ -21,6 +21,7 @@ from uuid import uuid4
from swift_testing import check_response, retry, skip, skip3, \
swift_test_perm, web_front_end
+from swift.common.utils import json
class TestObject(unittest.TestCase):
@@ -619,6 +620,117 @@ class TestObject(unittest.TestCase):
self.assertEquals(resp.read(), 'Invalid UTF8 or contains NULL')
self.assertEquals(resp.status, 412)
+ def test_cors(self):
+ if skip:
+ raise SkipTest
+
+ def is_strict_mode(url, token, parsed, conn):
+ conn.request('GET', '/info')
+ resp = conn.getresponse()
+ if resp.status // 100 == 2:
+ info = json.loads(resp.read())
+ return info.get('swift', {}).get('strict_cors_mode', False)
+ return False
+
+ def put_cors_cont(url, token, parsed, conn, orig):
+ conn.request(
+ 'PUT', '%s/%s' % (parsed.path, self.container),
+ '', {'X-Auth-Token': token,
+ 'X-Container-Meta-Access-Control-Allow-Origin': orig})
+ return check_response(conn)
+
+ def put_obj(url, token, parsed, conn, obj):
+ conn.request(
+ 'PUT', '%s/%s/%s' % (parsed.path, self.container, obj),
+ 'test', {'X-Auth-Token': token})
+ return check_response(conn)
+
+ def check_cors(url, token, parsed, conn,
+ method, obj, headers):
+ if method != 'OPTIONS':
+ headers['X-Auth-Token'] = token
+ conn.request(
+ method, '%s/%s/%s' % (parsed.path, self.container, obj),
+ '', headers)
+ return conn.getresponse()
+
+ strict_cors = retry(is_strict_mode)
+
+ resp = retry(put_cors_cont, '*')
+ resp.read()
+ self.assertEquals(resp.status // 100, 2)
+
+ resp = retry(put_obj, 'cat')
+ resp.read()
+ self.assertEquals(resp.status // 100, 2)
+
+ resp = retry(check_cors,
+ 'OPTIONS', 'cat', {'Origin': 'http://m.com'})
+ self.assertEquals(resp.status, 401)
+
+ resp = retry(check_cors,
+ 'OPTIONS', 'cat',
+ {'Origin': 'http://m.com',
+ 'Access-Control-Request-Method': 'GET'})
+
+ self.assertEquals(resp.status, 200)
+ resp.read()
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertEquals(headers.get('access-control-allow-origin'),
+ '*')
+
+ resp = retry(check_cors,
+ 'GET', 'cat', {'Origin': 'http://m.com'})
+ self.assertEquals(resp.status, 200)
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertEquals(headers.get('access-control-allow-origin'),
+ '*')
+
+ resp = retry(check_cors,
+ 'GET', 'cat', {'Origin': 'http://m.com',
+ 'X-Web-Mode': 'True'})
+ self.assertEquals(resp.status, 200)
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertEquals(headers.get('access-control-allow-origin'),
+ '*')
+
+ ####################
+
+ resp = retry(put_cors_cont, 'http://secret.com')
+ resp.read()
+ self.assertEquals(resp.status // 100, 2)
+
+ resp = retry(check_cors,
+ 'OPTIONS', 'cat',
+ {'Origin': 'http://m.com',
+ 'Access-Control-Request-Method': 'GET'})
+ resp.read()
+ self.assertEquals(resp.status, 401)
+
+ if strict_cors:
+ resp = retry(check_cors,
+ 'GET', 'cat', {'Origin': 'http://m.com'})
+ resp.read()
+ self.assertEquals(resp.status, 200)
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertTrue('access-control-allow-origin' not in headers)
+
+ resp = retry(check_cors,
+ 'GET', 'cat', {'Origin': 'http://secret.com'})
+ resp.read()
+ self.assertEquals(resp.status, 200)
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertEquals(headers.get('access-control-allow-origin'),
+ 'http://secret.com')
+ else:
+ resp = retry(check_cors,
+ 'GET', 'cat', {'Origin': 'http://m.com'})
+ resp.read()
+ self.assertEquals(resp.status, 200)
+ headers = dict((k.lower(), v) for k, v in resp.getheaders())
+ self.assertEquals(headers.get('access-control-allow-origin'),
+ 'http://m.com')
+
if __name__ == '__main__':
unittest.main()
diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py
index 115063f261..497cdcdd14 100644
--- a/test/unit/proxy/test_server.py
+++ b/test/unit/proxy/test_server.py
@@ -3805,9 +3805,7 @@ class TestObjectController(unittest.TestCase):
req.content_length = 0
resp = controller.OPTIONS(req)
self.assertEquals(200, resp.status_int)
- self.assertEquals(
- 'https://bar.baz',
- resp.headers['access-control-allow-origin'])
+ self.assertEquals('*', resp.headers['access-control-allow-origin'])
for verb in 'OPTIONS COPY GET POST PUT DELETE HEAD'.split():
self.assertTrue(
verb in resp.headers['access-control-allow-methods'])
@@ -3823,10 +3821,11 @@ class TestObjectController(unittest.TestCase):
def stubContainerInfo(*args):
return {
'cors': {
- 'allow_origin': 'http://foo.bar'
+ 'allow_origin': 'http://not.foo.bar'
}
}
controller.container_info = stubContainerInfo
+ controller.app.strict_cors_mode = False
def objectGET(controller, req):
return Response(headers={
@@ -3857,6 +3856,50 @@ class TestObjectController(unittest.TestCase):
'x-trans-id', 'x-object-meta-color'])
self.assertEquals(expected_exposed, exposed)
+ controller.app.strict_cors_mode = True
+ req = Request.blank(
+ '/v1/a/c/o.jpg',
+ {'REQUEST_METHOD': 'GET'},
+ headers={'Origin': 'http://foo.bar'})
+
+ resp = cors_validation(objectGET)(controller, req)
+
+ self.assertEquals(200, resp.status_int)
+ self.assertTrue('access-control-allow-origin' not in resp.headers)
+
+ def test_CORS_valid_with_obj_headers(self):
+ with save_globals():
+ controller = proxy_server.ObjectController(self.app, 'a', 'c', 'o')
+
+ def stubContainerInfo(*args):
+ return {
+ 'cors': {
+ 'allow_origin': 'http://foo.bar'
+ }
+ }
+ controller.container_info = stubContainerInfo
+
+ def objectGET(controller, req):
+ return Response(headers={
+ 'X-Object-Meta-Color': 'red',
+ 'X-Super-Secret': 'hush',
+ 'Access-Control-Allow-Origin': 'http://obj.origin',
+ 'Access-Control-Expose-Headers': 'x-trans-id'
+ })
+
+ req = Request.blank(
+ '/v1/a/c/o.jpg',
+ {'REQUEST_METHOD': 'GET'},
+ headers={'Origin': 'http://foo.bar'})
+
+ resp = cors_validation(objectGET)(controller, req)
+
+ self.assertEquals(200, resp.status_int)
+ self.assertEquals('http://obj.origin',
+ resp.headers['access-control-allow-origin'])
+ self.assertEquals('x-trans-id',
+ resp.headers['access-control-expose-headers'])
+
def _gather_x_container_headers(self, controller_call, req, *connect_args,
**kwargs):
header_list = kwargs.pop('header_list', ['X-Container-Device',
@@ -4824,9 +4867,7 @@ class TestContainerController(unittest.TestCase):
req.content_length = 0
resp = controller.OPTIONS(req)
self.assertEquals(200, resp.status_int)
- self.assertEquals(
- 'https://bar.baz',
- resp.headers['access-control-allow-origin'])
+ self.assertEquals('*', resp.headers['access-control-allow-origin'])
for verb in 'OPTIONS GET POST PUT DELETE HEAD'.split():
self.assertTrue(
verb in resp.headers['access-control-allow-methods'])