From 357b12dc2ba7b19c66196a573ccb2489d2104b93 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Fri, 16 Nov 2012 17:05:37 -0800 Subject: [PATCH] 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 --- doc/source/overview_container_sync.rst | 12 ---------- swift/common/middleware/keystoneauth.py | 8 +------ swift/common/middleware/tempauth.py | 8 +------ test/unit/common/middleware/test_tempauth.py | 23 -------------------- 4 files changed, 2 insertions(+), 49 deletions(-) diff --git a/doc/source/overview_container_sync.rst b/doc/source/overview_container_sync.rst index f0ccd44798..af0168791a 100644 --- a/doc/source/overview_container_sync.rst +++ b/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 ---------------------------------------------------------- diff --git a/swift/common/middleware/keystoneauth.py b/swift/common/middleware/keystoneauth.py index 67274cd3bb..af5914b70a 100644 --- a/swift/common/middleware/keystoneauth.py +++ b/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 diff --git a/swift/common/middleware/tempauth.py b/swift/common/middleware/tempauth.py index eb49b5f258..25597ed93b 100644 --- a/swift/common/middleware/tempauth.py +++ b/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 diff --git a/test/unit/common/middleware/test_tempauth.py b/test/unit/common/middleware/test_tempauth.py index 241035b978..7f5804d72f 100644 --- a/test/unit/common/middleware/test_tempauth.py +++ b/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')