Browse Source

Remove IP-based container-sync ACLs from auth middlewares.

The determination of the client IP looked at the X-Cluster-Client-Ip
and X-Forwarded-For headers in the incoming HTTP request. This is
trivially spoofable by a malicious client, so there's no security
gained by having the check there.

Worse, having the check there provides a false sense of security to
cluster operators. It sounds like it's based on the client IP, so an
attacker would have to do IP spoofing to defeat it. However, it's
really just a shared secret, and there's already a secret key set
up. Basically, it looks like 2-factor auth (IP+key), but it's really
1-factor (key).

Now, the one case where this might provide some security is where the
Swift cluster is behind an external load balancer that strips off the
X-Cluster-Client-Ip and X-Forwarded-For headers and substitutes its
own. I don't think it's worth the tradeoff, hence this commit.

Fixes bug 1068420 for very small values of "fixes".

DocImpact

Change-Id: I2bef64c2e1e4df8a612a5531a35721202deb6964
changes/58/16358/2
Samuel Merritt 9 years ago
parent
commit
357b12dc2b
  1. 12
      doc/source/overview_container_sync.rst
  2. 8
      swift/common/middleware/keystoneauth.py
  3. 8
      swift/common/middleware/tempauth.py
  4. 23
      test/unit/common/middleware/test_tempauth.py

12
doc/source/overview_container_sync.rst

@ -60,18 +60,6 @@ Additionally, it should be noted there is no way for an end user to detect sync
progress or problems other than HEADing both containers and comparing the
overall information.
The authentication system also needs to be configured to allow synchronization
requests. Here is an example with TempAuth::
[filter:tempauth]
# This is a comma separated list of hosts allowed to send
# X-Container-Sync-Key requests.
# allowed_sync_hosts = 127.0.0.1
allowed_sync_hosts = host1,host2,etc.
The default of 127.0.0.1 is just so no configuration is required for SAIO
setups -- for testing.
----------------------------------------------------------
Using the ``swift`` tool to set up synchronized containers
----------------------------------------------------------

8
swift/common/middleware/keystoneauth.py

@ -92,9 +92,6 @@ class KeystoneAuth(object):
'ResellerAdmin')
config_is_admin = conf.get('is_admin', "false").lower()
self.is_admin = swift_utils.config_true_value(config_is_admin)
cfg_synchosts = conf.get('allowed_sync_hosts', '127.0.0.1')
self.allowed_sync_hosts = [h.strip() for h in cfg_synchosts.split(',')
if h.strip()]
config_overrides = conf.get('allow_overrides', 't').lower()
self.allow_overrides = swift_utils.config_true_value(config_overrides)
@ -250,10 +247,7 @@ class KeystoneAuth(object):
if (req.environ.get('swift_sync_key')
and req.environ['swift_sync_key'] ==
req.headers.get('x-container-sync-key', None)
and 'x-timestamp' in req.headers
and (req.remote_addr in self.allowed_sync_hosts
or swift_utils.get_remote_client(req)
in self.allowed_sync_hosts)):
and 'x-timestamp' in req.headers):
log_msg = 'allowing proxy %s for container-sync' % req.remote_addr
self.logger.debug(log_msg)
return True

8
swift/common/middleware/tempauth.py

@ -84,10 +84,6 @@ class TempAuth(object):
if self.auth_prefix[-1] != '/':
self.auth_prefix += '/'
self.token_life = int(conf.get('token_life', 86400))
self.allowed_sync_hosts = [
h.strip()
for h in conf.get('allowed_sync_hosts', '127.0.0.1').split(',')
if h.strip()]
self.allow_overrides = config_true_value(
conf.get('allow_overrides', 't'))
self.storage_url_scheme = conf.get('storage_url_scheme', 'default')
@ -263,9 +259,7 @@ class TempAuth(object):
if (req.environ.get('swift_sync_key') and
req.environ['swift_sync_key'] ==
req.headers.get('x-container-sync-key', None) and
'x-timestamp' in req.headers and
(req.remote_addr in self.allowed_sync_hosts or
get_remote_client(req) in self.allowed_sync_hosts)):
'x-timestamp' in req.headers):
return None
if req.method == 'OPTIONS':
#allow OPTIONS requests to proceed as normal

23
test/unit/common/middleware/test_tempauth.py

@ -472,17 +472,6 @@ class TestAuth(unittest.TestCase):
self.assertEquals(resp.headers['x-storage-url'],
'fake://somehost:5678/v1/AUTH_test')
def test_allowed_sync_hosts(self):
a = auth.filter_factory({'super_admin_key': 'supertest'})(FakeApp())
self.assertEquals(a.allowed_sync_hosts, ['127.0.0.1'])
a = auth.filter_factory(
{'super_admin_key': 'supertest',
'allowed_sync_hosts':
'1.1.1.1,2.1.1.1, 3.1.1.1 , 4.1.1.1,, , 5.1.1.1'})(FakeApp())
self.assertEquals(
a.allowed_sync_hosts,
['1.1.1.1', '2.1.1.1', '3.1.1.1', '4.1.1.1', '5.1.1.1'])
def test_reseller_admin_is_owner(self):
orig_authorize = self.test_auth.authorize
owner_values = []
@ -593,18 +582,6 @@ class TestAuth(unittest.TestCase):
resp = req.get_response(self.test_auth)
self.assertEquals(resp.status_int, 401)
def test_sync_request_fail_sync_host(self):
self.test_auth.app = FakeApp(iter([('204 No Content', {}, '')]),
sync_key='secret')
req = self._make_request(
'/v1/AUTH_cfa/c/o',
environ={'REQUEST_METHOD': 'DELETE'},
headers={'x-container-sync-key': 'secret',
'x-timestamp': '123.456'})
req.remote_addr = '127.0.0.2'
resp = req.get_response(self.test_auth)
self.assertEquals(resp.status_int, 401)
def test_sync_request_success_lb_sync_host(self):
self.test_auth.app = FakeApp(iter([('204 No Content', {}, '')]),
sync_key='secret')

Loading…
Cancel
Save